Fix LocalExecutionService ignored errors #351

vinjana · 2020-10-07T11:03:38Z

LocalExecutionService ignored errors when running asynchronously with waitFor=false. This may have been responsible for non-detection of errors during bresume calls and accumulation of PSUPS LSF-cluster-jobs.

This fix uses the AsyncExecutionResult to wrap asynchronously computed exit code, stdout, & stderr. Currently, errors are only reported via log-messages if ignoreErrors = true but produce an UnexpectedExecutionResultException if ignoreErrors = false.

Additional smaller changes are commented in the PR.

… error reporting (via logger). Some smaller fixes and refactorings.

…3.5.9.

RoddyCore/src/de/dkfz/roddy/execution/io/ExecutionService.groovy

vinjana · 2020-10-07T11:05:46Z

RoddyCore/src/de/dkfz/roddy/execution/io/LocalExecutionService.groovy


 /**
 * The local execution service executes commands on the local machine. For this groovy's execute() method is used.
- * @author michael
+ * All commands are executed using Bash as interpreter!


This was not the case before. The change corrects the inconsistent behaviour.

That sounds like a pretty important (breaking?) change.
Should the comment permanently reflect this, e.g. by adding some wording like

prior to version vX.X.X, varying interpreters were used. This was normalised to consistently use bash starting at version

Good point. I also did not have updated the changelogs. This should be fixed. The choice to do it like this is justified by that for synchronous execution also bash -c is used (via LocalExecutionHelper in RoddyToolLib). Note that Bash is a requirement of Roddy anyway and I know only of executions in Bash environments. So this change is very unlikely to actually break anything.

Note however, that the SSHExecutionService apparently does not wrap everything into Bash. This may also be a good argument for removing the bash -c wrapping from RoddyToolLib. I'm uncertain. From a conceptual perspective the actual execution and the wrapping in Bash are different things and should be dealt with by different classes (e.g. some kind of BashCommand class that could also do basic syntax checks etc.). Also this may be an argument for actually not doing the wrapping at all. Unfortunately, I am not sure whether actually something would break. So wrapping everything into Bash is the safer solution than removing the wrapping from commands that possibly would break.

What is your opinion?

See the comparison of String.execute() and bash -c in a ProcessBuilder, below.

Not sure what the thumb-up means. "Good discussion"? "Leave it like it is"? "Poor guy. Keep up the good mood!" ;-)

RoddyCore/src/de/dkfz/roddy/execution/io/LocalExecutionService.groovy

vinjana · 2020-10-07T11:08:02Z

RoddyCore/src/de/dkfz/roddy/execution/io/fs/FileSystemAccessProvider.groovy

@@ -630,7 +630,7 @@ class FileSystemAccessProvider {
    boolean setAccessRightsRecursively(File path, String accessStringDirectories, String accessString, String group) {
        return ExecutionService.instance.
                execute(commandSet.getSetAccessRightsRecursivelyCommand(
-                        path, accessStringDirectories, accessString, group), false)
+                        path, accessStringDirectories, accessString, group))


This will continue to run asynchronously, but with better error reporting.

RoddyCore/src/de/dkfz/roddy/execution/jobs/Job.groovy

vinjana · 2020-10-07T11:08:54Z

build.gradle

@@ -92,7 +92,7 @@ repositories {
 }


-ext.roddyToolLibVersion = "2.1.1"
+ext.roddyToolLibVersion = "2.2.0"


See TheRoddyWMS/RoddyToolLib#27

RoddyCore/src/de/dkfz/roddy/plugins/LibrariesFactory.groovy

juleskers

Comments from yesterday, round 1

Still checking the rest together with @jpalbrecht

juleskers · 2020-10-08T14:04:13Z

Roddy.iml

@@ -1,81 +0,0 @@
-<?xml version="1.0" encoding="UTF-8"?>


Consistency: Why delete the .iml?
Comparing to RoddyToolLib, there .idea is gone, but the .iml is kept.

Actually, I did not notice and I am very surprised, because apparently IntelliJ does not need this. The file is gone, but the project module -- now simply imported from the Gradle model -- is present. Not sure yet, what that might mean.

I am more or less the only developer left and the system needed to be changed, because IntelliJ apparently does not support the old way with one module including sources and tests. All this is now automatically modelled as submodules and there is no way to change this, because IntelliJ automatically reimports from the Gradle-file.

I suggest we leave this like it is here in the PR.

RoddyCore/src/de/dkfz/roddy/execution/io/ExecutionService.groovy

juleskers · 2020-10-08T14:06:43Z

RoddyCore/src/de/dkfz/roddy/execution/io/LocalExecutionService.groovy

 import de.dkfz.roddy.tools.LoggerWrapper
+import groovy.transform.CompileStatic
+
+import java.util.concurrent.Callable


Apparently an unused import, according to IntelliJ?

RoddyCore/src/de/dkfz/roddy/execution/io/ExecutionService.groovy

juleskers · 2020-10-08T14:10:59Z

RoddyCore/src/de/dkfz/roddy/execution/io/LocalExecutionService.groovy


 /**
 * The local execution service executes commands on the local machine. For this groovy's execute() method is used.
- * @author michael
+ * All commands are executed using Bash as interpreter!


That sounds like a pretty important (breaking?) change.
Should the comment permanently reflect this, e.g. by adding some wording like

prior to version vX.X.X, varying interpreters were used. This was normalised to consistently use bash starting at version

RoddyCore/src/de/dkfz/roddy/execution/io/LocalExecutionService.groovy

juleskers · 2020-10-09T12:59:59Z

RoddyCore/test/src/de/dkfz/roddy/execution/io/LocalExecutionServiceSpec.groovy

+
+    def "execute('echo')"() {
+        when:
+        ExecutionResult res = es.execute("echo 'hello'", true)


For consistency compared to the methods below, should there be a waitFor = true in the test name (also for the next)

RoddyCore/test/src/de/dkfz/roddy/execution/io/LocalExecutionServiceSpec.groovy

juleskers · 2020-10-09T13:04:41Z

RoddyCore/test/src/de/dkfz/roddy/execution/io/LocalExecutionServiceSpec.groovy

+
+    LocalExecutionService es = ExecutionService.instance
+
+    def "execute('echo')"() {


As discussed in meeting: these names can be better 😄

RoddyCore/src/de/dkfz/roddy/execution/io/ExecutionService.groovy

vinjana · 2020-10-09T13:45:48Z

RoddyCore/src/de/dkfz/roddy/execution/io/LocalExecutionService.groovy

 import de.dkfz.roddy.tools.LoggerWrapper
+import groovy.transform.CompileStatic
+
+import java.util.concurrent.Callable


vinjana · 2020-10-09T13:52:12Z

RoddyCore/src/de/dkfz/roddy/execution/io/LocalExecutionService.groovy


 /**
 * The local execution service executes commands on the local machine. For this groovy's execute() method is used.
- * @author michael
+ * All commands are executed using Bash as interpreter!


Good point. I also did not have updated the changelogs. This should be fixed. The choice to do it like this is justified by that for synchronous execution also bash -c is used (via LocalExecutionHelper in RoddyToolLib). Note that Bash is a requirement of Roddy anyway and I know only of executions in Bash environments. So this change is very unlikely to actually break anything.

Note however, that the SSHExecutionService apparently does not wrap everything into Bash. This may also be a good argument for removing the bash -c wrapping from RoddyToolLib. I'm uncertain. From a conceptual perspective the actual execution and the wrapping in Bash are different things and should be dealt with by different classes (e.g. some kind of BashCommand class that could also do basic syntax checks etc.). Also this may be an argument for actually not doing the wrapping at all. Unfortunately, I am not sure whether actually something would break. So wrapping everything into Bash is the safer solution than removing the wrapping from commands that possibly would break.

RoddyCore/test/src/de/dkfz/roddy/execution/io/LocalExecutionServiceSpec.groovy

vinjana · 2020-10-09T14:24:32Z

RoddyCore/src/de/dkfz/roddy/execution/io/LocalExecutionService.groovy


 /**
 * The local execution service executes commands on the local machine. For this groovy's execute() method is used.
- * @author michael
+ * All commands are executed using Bash as interpreter!


…d but doesn't work anymore, but openjdk8 works.

…thub.com/TheRoddyWMS/Roddy into fix-localexecutionservice-ignores-errors

…and composition in BashCommandSet. - Accompanying changes to express optionality of return value in associated methods and handling of these changes. - Removed unnecessarily complicated Optional usage where simple if/else is better - Temporarily set RoddyToolLib dependency to test-release number for testing, until upstream changes are accepted - Fix unpredictable result line order for local synchronous execution tests. Tests were done with with without --include-build parameters for gradle and `gradle dependencies` to check that the right RoddyToolLib was included, to make sure, RTL and BE 0.0.7 play well together.

Whether it generally is a good idea to produce commands (like `rm -rf $file`), where the file can actually be a glob, one can be sceptical about. Some futher comments and small Roddy-internal refactorings.

…cCacheFileCommand. Rename a Spock test file. Otherwise layout and comment changes.

vinjana · 2020-10-19T13:18:57Z

RoddyCore/src/de/dkfz/roddy/execution/io/LocalExecutionService.groovy

-                command.execute()
-            }
-            return new ExecutionResult(true, 0, [], "")
+            ProcessBuilder processBuilder = new ProcessBuilder(["bash", "-c", command])


The old implementation used String.execute(String), which eventually refers to Runtime.exec(String command, String[] end, File dir). This latter method tokenizes the command using the delimiters space, tab, newline, carriage-return, form-feed. Bash, by contrast, uses the delimiters space, tab, newline by default (the $IFS variable), which, however, in this context here boils down to the same, because here only spaces are relevant token-delimiters. This means that in our context, command.execute() and bash -c ... called via ProcessBuilder will do the same.

vinjana · 2020-10-19T13:19:35Z

RoddyCore/src/de/dkfz/roddy/execution/io/LocalExecutionService.groovy


 /**
 * The local execution service executes commands on the local machine. For this groovy's execute() method is used.
- * @author michael
+ * All commands are executed using Bash as interpreter!


See the comparison of String.execute() and bash -c in a ProcessBuilder, below.

RoddyCore/src/de/dkfz/roddy/execution/io/fs/BashCommandSet.groovy

RoddyCore/src/de/dkfz/roddy/execution/io/fs/FileSystemAccessProvider.groovy

jpalbrecht · 2020-10-21T13:03:04Z

RoddyCore/test/src/de/dkfz/roddy/execution/io/LocalExecutionServiceSpec.groovy

    }

-    def "execute('echo \"hello\"; false', waitFor = false)"() {
+    def "execute synchronously and succeed with empty stdout and non!-ignored stderr"() {


shouldn't that be a "execute synchronously and fail " (exit code = 1)?

Sorry. Maybe a tabular test would have been more appropriate for all these parameter combinations and results.

RoddyCore/test/src/de/dkfz/roddy/execution/io/LocalExecutionServiceSpec.groovy

juleskers · 2020-10-21T13:11:28Z

Question: There is a seemingly empty merge-commit in the history.
Is that an accidental artifact?

OK either way if you keep or remove.

README.md

RoddyCore/src/de/dkfz/roddy/execution/io/fs/BashCommandSet.groovy

RoddyCore/src/de/dkfz/roddy/execution/io/fs/FileSystemAccessProvider.groovy

jpalbrecht · 2020-10-21T14:10:58Z

RoddyCore/src/de/dkfz/roddy/execution/io/fs/BashCommandSet.groovy

+    @Override
+    String getCheckChangeOfPermissionsPossibilityCommand(File file, String group) {
+        File testFile = new File(file, ".roddyPermissionsTestFile")
+        if (null == group) {


is Yoda conditions default in Roddy? Seen this at various positions.

I try to use these "Yoda conditions" (thanks for the term) to not by chance write group = null. Note that group == null evaluates to true if group is null, but group = null evaluates to false. So, in case of a typo with = rather than ==, not only group would be assigned, but also the other branch would be taken. Just a safety measure.

Actually, I never checked, but apparently Groovy does not accept assignments in conditions. So it is not strictly unnecessary. However, the "added cognitive load" counter-argument on the Wiki page is ridiculously irrelevant in the context of the overall complexity of the code. So I leave it as it is but avoid it in the future. Thanks for pointing out.

juleskers

Pair-reviewed together with @jpalbrecht ;

Good to merge, excluding a few minor remarks, and one incorrect spec-name (copy paste error);
All the other things are basically stylistic things that are also fine as-is.

juleskers · 2020-10-21T13:14:29Z

RoddyCore/src/de/dkfz/roddy/execution/io/ExecutionService.groovy

-                            provider.setDefaultAccessRightsRecursively(new File(analysisToolsServerDir.getAbsolutePath()), context)
+                            boolean success = provider.setDefaultAccessRightsRecursively(new File(analysisToolsServerDir.getAbsolutePath()), context)
+                            if (!success)
+                                logger.warning("Command error occurred and was ignored")


This message is the equivalent of "something went wrong" in it's informativeness.

Alternative suggestion:
"Ignoring command error while setting default access rights on analysisToolsServerDir"

Too true to be true.

RoddyCore/src/de/dkfz/roddy/execution/io/ExecutionService.groovy

- recompile for 2.5.9 release including timestamping - some code reformating (to be able to read on my narrow notebook screen) - test class rename to be standard-conform

…fixed it.

vinjana added 3 commits October 7, 2020 12:53

Idea config updates.

bc81e7e

LocalExecutionService now allows for asynchronous execution including…

c48e8b5

… error reporting (via logger). Some smaller fixes and refactorings.

Revert change in FSAP (async change of permissions). Bump version to …

f9e5c0f

…3.5.9.

vinjana commented Oct 7, 2020

View reviewed changes

vinjana requested review from askask and removed request for askask October 7, 2020 11:14

juleskers reviewed Oct 9, 2020

View reviewed changes

Answers to reviewers.

174a97a

vinjana commented Oct 9, 2020

View reviewed changes

vinjana and others added 2 commits October 9, 2020 17:29

Moved from sdkman! JDK to travis jdk feature. oraclejdk8 is documente…

5c3681e

…d but doesn't work anymore, but openjdk8 works.

Update README.md

6862f82

vinjana requested a review from juleskers October 12, 2020 11:13

vinjana added 5 commits October 12, 2020 13:25

Comment change.

0b507e3

Merge branch 'fix-localexecutionservice-ignores-errors' of https://gi…

73a0ccd

…thub.com/TheRoddyWMS/Roddy into fix-localexecutionservice-ignores-errors

Removed some quotes around filenames to conform to the rest of the code.

b096502

Whether it generally is a good idea to produce commands (like `rm -rf $file`), where the file can actually be a glob, one can be sceptical about. Some futher comments and small Roddy-internal refactorings.

Use absolute path for parent directory in getCheckCreateAndReadoutExe…

5fc30bc

…cCacheFileCommand. Rename a Spock test file. Otherwise layout and comment changes.

vinjana commented Oct 19, 2020

View reviewed changes

jpalbrecht reviewed Oct 21, 2020

View reviewed changes

juleskers reviewed Oct 21, 2020

View reviewed changes

RoddyCore/test/src/de/dkfz/roddy/execution/io/LocalExecutionServiceSpec.groovy Show resolved Hide resolved

jpalbrecht reviewed Oct 21, 2020

View reviewed changes

juleskers approved these changes Oct 21, 2020

View reviewed changes

vinjana added 4 commits October 21, 2020 17:14

Improved error messages

fd7e18b

- recompile for 2.5.9 release including timestamping - some code reformating (to be able to read on my narrow notebook screen) - test class rename to be standard-conform

Set RoddyToolLib dependency to 2.2.1.

f742da7

Added RNA-seq workflow to workflow list in README.

e47153b

Improved LocalExecutionServiceSpec, discovered RoddyToolLib bug, and …

6c5c53d

…fixed it.

vinjana merged commit e735922 into master Oct 23, 2020

vinjana deleted the fix-localexecutionservice-ignores-errors branch October 23, 2020 07:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix LocalExecutionService ignored errors #351

Fix LocalExecutionService ignored errors #351

vinjana commented Oct 7, 2020

vinjana Oct 7, 2020

juleskers Oct 8, 2020

vinjana Oct 9, 2020

vinjana Oct 9, 2020

vinjana Oct 12, 2020

vinjana Oct 19, 2020

vinjana Oct 21, 2020

vinjana Oct 7, 2020

vinjana Oct 7, 2020

juleskers left a comment

juleskers Oct 8, 2020

vinjana Oct 12, 2020

vinjana Oct 12, 2020

juleskers Oct 8, 2020

vinjana Oct 9, 2020

juleskers Oct 8, 2020

juleskers Oct 9, 2020

juleskers Oct 9, 2020

vinjana Oct 9, 2020

vinjana Oct 9, 2020

vinjana Oct 9, 2020

vinjana Oct 19, 2020

vinjana Oct 19, 2020

jpalbrecht Oct 21, 2020 •

edited

Loading

vinjana Oct 21, 2020

juleskers commented Oct 21, 2020

jpalbrecht Oct 21, 2020

vinjana Oct 21, 2020

vinjana Oct 21, 2020

juleskers left a comment

juleskers Oct 21, 2020

vinjana Oct 21, 2020


		LocalExecutionService es = ExecutionService.instance

		def "execute('echo')"() {

Fix LocalExecutionService ignored errors #351

Fix LocalExecutionService ignored errors #351

Conversation

vinjana commented Oct 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juleskers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpalbrecht Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juleskers commented Oct 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juleskers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpalbrecht Oct 21, 2020 •

edited

Loading