-
Notifications
You must be signed in to change notification settings - Fork 735
Separate CLI from runtime #5971
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
This comment was marked as off-topic.
This comment was marked as off-topic.
Due to the nature of this PR, I will keep it updated via rebase instead of merge commits so that the changes are as clear as possible. |
oh mamma mia 😆 |
Your k8s plugin inspired me 😄 |
I feared that 😆 |
Jokes aside, it might be good to do the actual code changes in a separate PR so that the nf-runtime part is as clean as possible The nf-runtime refactor was just the easiest way to reveal the circular dependencies |
I'll review post 25.04 |
Refactored so that the CLI code is in a new module Note the following plugins depend explicitly on the CLI code because they add CLI commands:
Might be possible to remove this dependency by moving some interfaces into the core runtime, but not a big deal either way |
ac8c183
to
bf822ef
Compare
b4b321e
to
069653d
Compare
5cc1731
to
fd1408d
Compare
I refactored the Couldn't quite do this for |
modules/nextflow/build.gradle
Outdated
api ('org.yaml:snakeyaml:2.2') | ||
api ('org.jsoup:jsoup:1.15.4') | ||
api 'org.iq80.leveldb:leveldb:0.12' | ||
api 'org.eclipse.jgit:org.eclipse.jgit:7.1.0.202411261347-r' |
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.
Was this downgraded on purpose? before it was 7.1.1.202505221757
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.
this is likely an incorrect merge commit, none of the versions should be changed. I will fix it
Did I break anything? I see some tests failing. Also trying locally I get
|
Actually that's my fault, looks like a regression from #6257 . I will submit a separate fix |
498fd1e
to
0faaf13
Compare
Found some possible solutions for decoupling nf-k8s, nf-tower, and nf-wave from CLI v1. See top comment for details. |
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
This PR splits the nextflow module into two modules:
nf-cli-v1
: contains only the Launcher and CLI classesnextflow
: contains everything (i.e. the "runtime")Benefits:
Notes:
I moved the CLI bits of
ConfigBuilder
into a separate classConfigCmdAdapter
in order to keep the core config builder in the runtime. This also provides a nice separation of concerns.I moved
K8sDriverLauncher
intonf-cli-v1
so that thenf-k8s
is CLI-agnostic. This means I have to included nf-k8s as a build-time dep for CLI v1, but CLI v2 will be able to loadnf-k8s
on-demand as a plugin.I moved
PluginExecAware
intonextflow
so that plugins can declare CLI commands without depending onnf-cli-v1
. I replaced the config loading here with a minimal config, but this might not be enough, so we might want to think about how much of the config logic should be replicated for plugin commands