Skip to content
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

testing fix for config variables #3432

Merged

Conversation

vsoch
Copy link
Contributor

@vsoch vsoch commented Nov 24, 2022

This is a tiny change to ensure that the flux.terminalOutput is correctly passed all the way from the config to the executor! For some reason with task.config, it's not there, likely because it's not known to the task to be parsed?

This was all @pditommaso thank you for the help! Once this is in we are good to go for Nextflow with Flux I think.

Here is the fixed workflow (before this said the job didn't produce output):

image

Signed-off-by: vsoch vsoch@users.noreply.github.com

@vsoch
Copy link
Contributor Author

vsoch commented Nov 24, 2022

Okay looks like the session is null (and likely needs to be init somehow)

Condition failed with Exception:

executor.getSubmitCommandLine(task, Paths.get('/some/path/job.sh')) == ['flux', 'mini', 'submit', '--setattr=cwd=/work/path', '--job-name="nf-my_task"', '--output=/work/path/.command.log', '--queue=delta', '/bin/bash', 'job.sh']
|        |                    |     |     |
|        |                    |     |     /some/path/job.sh
|        |                    |     class java.nio.file.Paths
|        |                    id: null; name: my task; type: null; exit: -; error: null; workDir: /work/path
|        java.lang.NullPointerException: Cannot get property 'config' on null object
|        	at nextflow.executor.FluxExecutor.getSubmitCommandLine(FluxExecutor.groovy:66)
|        	at nextflow.executor.FluxExecutorTest.testGetCommandLine(FluxExecutorTest.groovy:71)
<nextflow.executor.FluxExecutor@5655a930 queueInterval=null lastQueueStatus=null session=null name=null monitor=null memoizedMethodClosure$getStageDir=org.codehaus.groovy.runtime.memoize.Memoize$MemoizeFunction@12234922>

	at nextflow.executor.FluxExecutorTest.testGetCommandLine(FluxExecutorTest.groovy:71)
Caused by: java.lang.NullPointerException: Cannot get property 'config' on null object
	at nextflow.executor.FluxExecutor.getSubmitCommandLine(FluxExecutor.groovy:66)
	... 1 more

@pditommaso
Copy link
Member

The rescue team is coming ..

@vsoch
Copy link
Contributor Author

vsoch commented Nov 24, 2022

Oh nice! I didn't know there was a rescue team! We have those in my town when people get lost or injured in the mountains. 😆

@vsoch
Copy link
Contributor Author

vsoch commented Nov 24, 2022

I'm seeing a few patterns / ideas that could be promising:

  • maybe we need to import nextflow.Session
  • we could try a pattern that checks if it's null first (something like session ? session ...) assuming sometimes it's null, other times not.

@pditommaso

This comment was marked as outdated.

@pditommaso
Copy link
Member

Hold on

@pditommaso
Copy link
Member

This is the correct one patch.txt

@vsoch
Copy link
Contributor Author

vsoch commented Nov 24, 2022

Both failed to apply, but I'll look at the changes and see if I can add them to the PR!

@vsoch
Copy link
Contributor Author

vsoch commented Nov 24, 2022

oh it seemed to take! not sure why I got an error message. Should we add something to the FluxExecutor when the session isn't defined, or will it always be defined?

@pditommaso
Copy link
Member

No, the session is expected to be defined during normal execution. It's in the test that a fake session needs to be injected

@vsoch
Copy link
Contributor Author

vsoch commented Nov 24, 2022

Ah perfect, thank you rescue crew! 🙌

Signed-off-by: vsoch <vsoch@users.noreply.github.com>
@vsoch
Copy link
Contributor Author

vsoch commented Nov 24, 2022

Going to bring up my devcontainer environment and be more responsible to actually try the tests before pushing here!

@pditommaso
Copy link
Member

Ok, green. Ready to merge it?

@vsoch
Copy link
Contributor Author

vsoch commented Nov 24, 2022

I think so - let me do one more sanity check just to be extra sure!

@vsoch
Copy link
Contributor Author

vsoch commented Nov 24, 2022

Looks great - ship it! 🚢

@pditommaso
Copy link
Member

@pditommaso pditommaso merged commit 68b45c9 into nextflow-io:master Nov 24, 2022
@vsoch vsoch deleted the fix/flux-config-variables branch November 24, 2022 20:35
@vsoch
Copy link
Contributor Author

vsoch commented Nov 24, 2022

I come for the code development, and I stay for the animated gifs! 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug translating nextflow.config nested "flux" variables into workflow
2 participants