-
Notifications
You must be signed in to change notification settings - Fork 726
Fix unstage controls in command.run when using storeDir #6364
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: jorgee <[email protected]>
✅ Deploy Preview for nextflow-docs-staging canceled.
|
Signed-off-by: jorgee <[email protected]>
@@ -160,6 +160,10 @@ class BashWrapperBuilder { | |||
return targetDir && workDir!=targetDir | |||
} | |||
|
|||
protected boolean shouldUnstageControls() { | |||
return false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this should not default to shouldUnstageOutputs()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the issue was caused because it was using shouldUnstageOutputs
in both outputs and controls. If we default to shouldUnstageOutputs
we will not solve the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure Jorge, especially this
However, control logs (.command.err, .command.out, etc.) are always copied to the working directory.
Only the .command.log
and .command.begin
and .exitcode
are created directly in the (target) work dir. The stage controls are these below and follow the usual logic as for output files
nextflow/modules/nextflow/src/main/groovy/nextflow/executor/BashWrapperBuilder.groovy
Lines 773 to 778 in e89b6e0
def result = copyFileToWorkDir(TaskRun.CMD_OUTFILE) + ' || true' + ENDL | |
result += copyFileToWorkDir(TaskRun.CMD_ERRFILE) + ' || true' + ENDL | |
if( statsEnabled ) | |
result += copyFileToWorkDir(TaskRun.CMD_TRACE) + ' || true' + ENDL | |
if( outputEnvNames || outputEvals ) | |
result += copyFileToWorkDir(TaskRun.CMD_ENV) + ' || true' + ENDL |
|
||
@Override | ||
protected boolean shouldUnstageControls() { | ||
return true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this only azure specific ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shouldUnstageOutputs
is also azure specific. It was always unstaging outputs and controls. In a first implementation, I removed the specific Azure behaviour for controls but tests were failing, so I decided to keep it.
close #6311
unstage_controls
was set in .command.run
using the same condition as the unstage outputs. It was producing an error message in.command.log
when using thestoreDir
. However, control logs (.command.err, .command.out, etc.) are always copied to the working directory.This PR changes the condition when the
unstage_controls
is set. A new methodshouldUnstageControls
is created to differentiate the condition when checking the outputs or controls. By default, it is set to false, the unstage controls will be applied if scratch is defined. There is a special case for Azure, where boths controls and outputs are forced to be unstaged