Skip to content

Commit

Permalink
Enable Checkstyle and FindBugs in TonY #166 (#171)
Browse files Browse the repository at this point in the history 
* Enable Checkstyle and FindBugs in TonY #166

* Fix test failures by adding back TaskUrl.compareTo
  • Loading branch information
@erwa
erwa authored Feb 3, 2019
1 parent 2a48fc5 commit 69bdf38
Show file tree
Hide file tree
Showing 29 changed files with 398 additions and 186 deletions.
22 changes: 22 additions & 0 deletions build.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,8 @@ subprojects {
apply plugin: "license"
apply plugin: "com.google.protobuf"
apply plugin: "java"
apply plugin: "checkstyle"
apply plugin: "findbugs"
apply plugin: "eclipse"
apply plugin: "idea"
apply plugin: "maven-publish"
Expand Down Expand Up @@ -180,6 +182,26 @@ subprojects {
signing {
sign publishing.publications.mavenJava
}

checkstyle {
configFile rootProject.file('config/checkstyle/checkstyle.xml')
maxWarnings 0
}

tasks.withType(FindBugs) {
reports {
xml.enabled false
html.enabled true
}

classpath = classpath.filter {
// POM files are getting included in the classpath for some reason, but this causes parsing errors when FindBugs
// tries to analyze them, thinking they are zips. Excluding POM files as a workaround.
!it.name.endsWith('.pom')
}

excludeFilter = rootProject.file('config/findbugs/findbugsExclude.xml')
}
}

apply plugin: 'distribution'
Expand Down
198 changes: 198 additions & 0 deletions config/checkstyle/checkstyle.xml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,198 @@
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE module PUBLIC "-//Puppy Crawl//DTD Check Configuration 1.3//EN" "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">

<!--
Checkstyle-Configuration: LinkedIn Style
Description:
LinkedIn Java style.
-->
<module name="Checker">
<property name="severity" value="warning"/>
<property name="fileExtensions" value="java"/>

<module name="TreeWalker">
<property name="tabWidth" value="2"/>
<module name="SuppressWarningsHolder"/>
<module name="FileContentsHolder"/>

<!-- ANNOTATIONS -->

<!-- No trailing empty parenthesis or commas -->
<module name="AnnotationUseStyle">
<property name="elementStyle" value="ignore"/>
</module>
<!-- Ensure @Override is present when {@inheritDoc} Javadoc tag is present -->
<module name="MissingOverride"/>
<!-- Package level annotations belong in package-info.java -->
<module name="PackageAnnotation"/>

<!-- BLOCKS -->

<!-- Block opening brace on same line -->
<module name="LeftCurly">
<property name="option" value="eol"/>
</module>
<!-- Block closing brace for else, catch, finally on same line -->
<module name="RightCurly">
<property name="option" value="same"/>
</module>
<!-- Always use braces even if optional -->
<module name="NeedBraces"/>

<!-- CLASS DESIGN -->

<!-- Classes containing only static methods should not have a public constructor -->
<module name="HideUtilityClassConstructor"/>

<!-- CODING -->

<!-- Use Java style array declarations (e.g. String[] names), not C style (e.g. String names[]) -->
<module name="ArrayTypeStyle"/>
<!-- If covariant equals defined, standard equals must also be defined -->
<module name="CovariantEquals"/>
<!-- Switch 'default' case must appear last -->
<module name="DefaultComesLast"/>
<!-- Override equals and hashCode together -->
<module name="EqualsHashCode"/>
<!-- No fall through in switch cases, even the last one -->
<module name="FallThrough">
<property name="checkLastCaseGroup" value="true"/>
</module>
<!-- Do not perform assignments embedded within expressions -->
<module name="InnerAssignment"/>
<!-- Switch statements must have a 'default' case -->
<module name="MissingSwitchDefault"/>
<!-- Do not modify the 'for' loop control variable -->
<module name="ModifiedControlVariable"/>
<!-- Each variable delcaration must be on a separate line -->
<module name="MultipleVariableDeclarations"/>
<!-- Each statement (i.e. code terminated by a semicolon) must be on a separate line -->
<module name="OneStatementPerLine"/>
<!-- Classes must have an explicit package declaration -->
<module name="PackageDeclaration"/>
<!-- Do not test boolean expressions against the values true or false -->
<module name="SimplifyBooleanExpression"/>
<!-- Do not test for boolean conditions and return the values true or false -->
<module name="SimplifyBooleanReturn"/>
<!-- Do not use '==' to compare string against a literal; use 'equals' -->
<module name="StringLiteralEquality"/>
<!-- Use 'L' with long literals -->
<module name="UpperEll"/>

<!-- IMPORTS -->

<!-- No imports statements using '*' notation except static imports -->
<module name="AvoidStarImport">
<property name="allowStaticMemberImports" value="true"/>
</module>
<!-- Do not import 'sun' packages -->
<module name="IllegalImport"/>
<!-- Do not duplicate import statements -->
<module name="RedundantImport"/>
<!-- Eliminate unused imports -->
<module name="UnusedImports"/>

<!-- JAVADOC COMMENTS -->

<!-- If you have a Javadoc comment, make sure it is properly formed -->
<module name="JavadocStyle">
<property name="checkFirstSentence" value="false"/>
</module>

<!-- NAMING CONVENTIONS -->

<!-- Generic parameters for a class must be uppercase letters separated by underscores (e.g. <V>, <NEW>, <KEY_T>) -->
<module name="ClassTypeParameterName">
<property name="format" value="^[A-Z]+(_[A-Z]+)*$"/>
</module>
<!-- Constants must be all uppercase letters separated by underscores -->
<module name="ConstantName">
<property name="format" value="^(_?log)|([A-Z][A-Z0-9]*(_[A-Z0-9]+)*)$"/>
</module>
<!-- Local variables must be camel case starting with lowercase letter -->
<module name="LocalFinalVariableName"/>
<module name="LocalVariableName"/>
<!-- Member variables must be camel case starting with an underscore or lowercase letter -->
<module name="MemberName">
<property name="format" value="^[_a-z][a-zA-Z0-9]*$"/>
</module>
<!-- Method name must be camel case starting with a lowercase letter -->
<module name="MethodName"/>
<!-- Generic parameters for a method must be uppercase letters separated by underscores (e.g. <V>, <NEW>, <KEY_T>) -->
<module name="MethodTypeParameterName">
<property name="format" value="^[A-Z]+(_[A-Z]+)*$"/>
</module>
<!-- Package name must be all lowercase letters separated by periods -->
<module name="PackageName">
<property name="format" value="^[a-z]+(\.[a-z][a-z0-9]*)*$"/>
</module>
<!-- Parameters must be camel case starting with a lowercase letter -->
<module name="ParameterName"/>
<!-- Static variables must be camel case starting with an underscore or lowercase letter -->
<module name="StaticVariableName">
<property name="format" value="^[_a-z][a-zA-Z0-9]*$"/>
</module>
<!-- Type names must be camel case starting with an uppercase letter -->
<module name="TypeName"/>

<!-- LENGTHS -->

<!-- Desired line length is 120 but allow some overrun beyond that -->
<module name="LineLength">
<property name="max" value="160"/>
<message key="maxLineLen" value="Line is longer than {0,number,integer} characters (found {1,number,integer}). Try to keep lines under 120 characters."/>
</module>

<!-- WHITESPACE -->

<module name="GenericWhitespace"/>
<module name="MethodParamPad"/>
<module name="NoWhitespaceAfter">
<property name="tokens" value="BNOT,DEC,DOT,INC,LNOT,UNARY_MINUS,UNARY_PLUS"/>
</module>
<module name="NoWhitespaceBefore"/>
<module name="OperatorWrap"/>
<module name="ParenPad"/>
<module name="TypecastParenPad">
<property name="tokens" value="RPAREN,TYPECAST"/>
</module>
<module name="WhitespaceAfter"/>
<module name="WhitespaceAround"/>

<!-- Do not allow meaningless, IDE generated parameter names -->
<module name="RegexpSinglelineJava">
<property name="format" value="[\s]+arg[\d]+[,\)]"/>
<property name="message" value="Replace argN with a meaningful parameter name"/>
</module>
</module>

<!-- Do not allow tab characters in source files -->
<module name="FileTabCharacter"/>

<!-- Ensure parameter and exception names are present on @param and @throws tags -->
<module name="RegexpSingleline">
<property name="format" value="\*[\s]*@(throws|param)[\s]*$"/>
<property name="message" value="Missing parameter or exception name"/>
</module>
<!-- IDE generated code must be reviewed by developer -->
<module name="RegexpSingleline">
<property name="format" value="\/\/[\s]*TODO[\s]+Auto-generated"/>
<property name="message" value="Replace IDE generated code with real implementation"/>
</module>
<!-- Detect commonly misspelled Javadoc tags -->
<module name="RegexpSingleline">
<property name="format" value="\*[\s]*@(params|throw|returns)[\s]+"/>
<property name="message" value="Correct misspelled Javadoc tag"/>
</module>

<!-- Read checker suppressions from a file -->
<module name="SuppressionFilter">
<property name="file" value="${config_loc}/suppressions.xml"/>
</module>
<!-- Allow Checkstyle warnings to be suppressed using trailing comments -->
<module name="SuppressWithNearbyCommentFilter"/>
<!-- Allow Checkstyle warnings to be suppressed using block comments -->
<module name="SuppressionCommentFilter"/>
<!-- Allow SuppressWarnings annotation to suppress Checkstyle issues -->
<module name="SuppressWarningsFilter"/>
</module>
7 changes: 7 additions & 0 deletions config/checkstyle/suppressions.xml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?xml version="1.0"?>
<!DOCTYPE suppressions PUBLIC
"-//Puppy Crawl//DTD Suppressions 1.1//EN"
"http://www.puppycrawl.com/dtds/suppressions_1_1.dtd">
<suppressions>
<suppress files="/generated/" checks=".*"/>
</suppressions>
5 changes: 5 additions & 0 deletions config/findbugs/findbugsExclude.xml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<FindBugsFilter>
<Match>
<Source name="~.*YarnTensorFlowClusterProtos.*java"/>
</Match>
</FindBugsFilter>
23 changes: 13 additions & 10 deletions tony-azkaban/src/main/java/com/linkedin/tony/azkaban/TensorFlowJob.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,40 +84,41 @@ protected String getJVMArguments() {

@Override
protected String getMainArguments() {
String args = super.getMainArguments();
StringBuilder args = new StringBuilder(super.getMainArguments());
info("All job props: " + getJobProps());
info("All sys props: " + getSysProps());
String srcDir = getJobProps().getString(TensorFlowJobArg.SRC_DIR.azPropName, "src");
args += " " + TensorFlowJobArg.SRC_DIR.tonyParamName + " " + srcDir;
args.append(" " + TensorFlowJobArg.SRC_DIR.tonyParamName + " " + srcDir);

String hdfsClasspath = getJobProps().getString(TensorFlowJobArg.HDFS_CLASSPATH.azPropName, null);
if (hdfsClasspath != null) {
args += " " + TensorFlowJobArg.HDFS_CLASSPATH.tonyParamName + " " + hdfsClasspath;
args.append(" " + TensorFlowJobArg.HDFS_CLASSPATH.tonyParamName + " " + hdfsClasspath);
}

Map<String, String> workerEnvs = getJobProps().getMapByPrefix(WORKER_ENV_PREFIX);
for (Map.Entry<String, String> workerEnv : workerEnvs.entrySet()) {
args += " " + TensorFlowJobArg.SHELL_ENV.tonyParamName + " " + workerEnv.getKey() + "=" + workerEnv.getValue();
args.append(" " + TensorFlowJobArg.SHELL_ENV.tonyParamName + " " + workerEnv.getKey()
+ "=" + workerEnv.getValue());
}

String taskParams = getJobProps().getString(TensorFlowJobArg.TASK_PARAMS.azPropName, null);
if (taskParams != null) {
args += " " + TensorFlowJobArg.TASK_PARAMS.tonyParamName + " '" + taskParams + "'";
args.append(" " + TensorFlowJobArg.TASK_PARAMS.tonyParamName + " '" + taskParams + "'");
}

String pythonBinaryPath = getJobProps().getString(TensorFlowJobArg.PYTHON_BINARY_PATH.azPropName, null);
if (pythonBinaryPath != null) {
args += " " + TensorFlowJobArg.PYTHON_BINARY_PATH.tonyParamName + " " + pythonBinaryPath;
args.append(" " + TensorFlowJobArg.PYTHON_BINARY_PATH.tonyParamName + " " + pythonBinaryPath);
}

String pythonVenv = getJobProps().getString(TensorFlowJobArg.PYTHON_VENV.azPropName, null);
if (pythonVenv != null) {
args += " " + TensorFlowJobArg.PYTHON_VENV.tonyParamName + " " + pythonVenv;
args.append(" " + TensorFlowJobArg.PYTHON_VENV.tonyParamName + " " + pythonVenv);
}

String executes = getJobProps().getString(TensorFlowJobArg.EXECUTES.azPropName, null);
if (executes != null) {
args += " " + TensorFlowJobArg.EXECUTES.tonyParamName + " " + executes;
args.append(" " + TensorFlowJobArg.EXECUTES.tonyParamName + " " + executes);
}

Map<String, String> tonyConfs = getJobProps().getMapByPrefix(TONY_CONF_PREFIX);
Expand All @@ -127,7 +128,9 @@ protected String getMainArguments() {
}

// Write user's tony confs to an xml to be localized.
tonyConfFile.getParentFile().mkdir();
if (!tonyConfFile.getParentFile().mkdir()) {
throw new RuntimeException("Failed to create parent directory for TonY conf file.");
}
try (OutputStream os = new FileOutputStream(tonyConfFile)) {
tfConf.writeXml(os);
} catch (Exception e) {
Expand All @@ -136,6 +139,6 @@ protected String getMainArguments() {

info("Complete main arguments: " + args);

return args;
return args.toString();
}
}
2 changes: 1 addition & 1 deletion tony-azkaban/src/test/java/com/linkedin/tony/azkaban/TestTensorFlowJob.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public class TestTensorFlowJob {
private static void initServiceProvider() {
final Injector injector = Guice.createInjector(new AbstractModule() {
@Override
protected void configure() {}
protected void configure() { }
});
// Because SERVICE_PROVIDER is a singleton and it is shared among many tests,
// need to reset the state to avoid assertion failures.
Expand Down
11 changes: 2 additions & 9 deletions tony-cli/src/main/java/com/linkedin/tony/cli/NotebookSubmitter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public int submit(String[] args) throws ParseException, URISyntaxException, IOEx
opts.addOption("src_dir", true, "Name of directory of source files.");

int exitCode = 0;
Path cachedLibPath = null;
Path cachedLibPath;
Configuration hdfsConf = new Configuration();
try (FileSystem fs = FileSystem.get(hdfsConf)) {
cachedLibPath = new Path(fs.getHomeDirectory(), TONY_FOLDER + Path.SEPARATOR + UUID.randomUUID().toString());
Expand All @@ -67,10 +67,7 @@ public int submit(String[] args) throws ParseException, URISyntaxException, IOEx
LOG.info("Copying " + jarPath + " to: " + cachedLibPath);
} catch (IOException e) {
LOG.fatal("Failed to create FileSystem: ", e);
exitCode = -1;
}
if (cachedLibPath == null) {
System.exit(-1);
return -1;
}

String[] updatedArgs = Arrays.copyOf(args, args.length + 4);
Expand All @@ -81,9 +78,6 @@ public int submit(String[] args) throws ParseException, URISyntaxException, IOEx

client.init(updatedArgs);
client.start();
if (client == null) {
System.exit(-1);
}
Thread clientThread = new Thread(client::start);
clientThread.start();
while (clientThread.isAlive()) {
Expand Down Expand Up @@ -118,6 +112,5 @@ public static void main(String[] args) throws Exception {
NotebookSubmitter submitter = new NotebookSubmitter();
exitCode = submitter.submit(args);
System.exit(exitCode);

}
}
4 changes: 2 additions & 2 deletions tony-core/src/main/java/com/linkedin/tony/TFConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ public static class Task {
private int index;

// Jackson needs a default constructor
Task() {}
Task() { }

Task(String type, int index) {
this.type = type;
Expand All @@ -47,7 +47,7 @@ public void setIndex(int index) {
}

// Jackson needs a default constructor
TFConfig() {}
TFConfig() { }

public TFConfig(Map<String, List<String>> clusterSpec, String jobName, int taskIndex) {
this.clusterSpec = clusterSpec;
Expand Down
Loading

0 comments on commit 69bdf38

Please sign in to comment.