Separate CLI from runtime - #5971
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 |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
498fd1e to
0faaf13
Compare
This comment was marked as resolved.
This comment was marked as resolved.
|
Spun out each "rider change" into separate PRs:
Marking this PR as draft until these three are merged, then I will update this PR accordingly |
c394425 to
b20d002
Compare
|
Thanks for splitting these out, Ben — and sorry for the extra round trips. The result speaks for itself: the diff is down to 103 files and reads as an actual mechanical move now. I went back through the spin-outs and they all check out:
The ADR is a genuinely good addition. Calling out the A few new things I picked up on this pass, none of them fundamental: Worth fixing before merge
Since nf-k8s is still distributed as a plugin too ( To be clear though, dissolving the Test heap for the
Nits
CIThe four failures are all 90-minute timeouts on Nothing here is structural. Happy to approve once the packaging and heap bits are sorted. |
|
No problem. This PR deserves plenty of scrutiny. Updates:
Regarding your concerns about I have updated the ADR with the split-package consequence. I will also perform some manual tests to make sure everything still works |
|
Tested The only edge case is if a user explicitly declares nf-k8s in config: plugins { id 'nf-k8s' }In this case, the config validator will warn about the |
|
Went through the updates and re-validated the parts that needed evidence. Everything from the last round is addressed — First, a correction to my last review. I claimed the split package could make plugin version pinning a silent no-op. It can't, and you were right to push back. I resolved the executor at runtime with both copies present: pf4j enumerates classpath extensions before plugin extensions, so the plugin copy always wins. Pinning holds and there's no cross-classloader hazard in practice.
|
|
pditommaso
left a comment
There was a problem hiding this comment.
All four addressed — re-validated on bfbc2e90f rather than taking them on trust:
- Rebuilt the fat jar and reran the case that was fatal before, with the published
nf-tower@1.29.1pulled from the registry. Plugin starts, command exits 0. Nonextflow.plugin.clireferences left anywhere, docs import updated too. - The
ConfigValidatorfix being keyed on the class name rather than special-casing nf-k8s is the right shape — genuine collisions between two different classes still warn. Confirmed zeroconflicts with existing scopelines onprocess.executor = 'k8s', withk8s.namespacestill validating. - On the
LoggerOptionsnamed args I checked propagation rather than just the compile, since a silent map-constructor mismatch under@CompileStaticwould have been invisible:-logwrites to the given path and-trace nextflow.pluginyields TRACE output. Both fine. - ADR reads accurately now on all of it.
Fair call on AuthCommand/LaunchCommand — it fails loudly and only costs those two commands, and core plugins are rebuilt at release anyway.
Thanks Ben, and thanks for the patience across all the rounds on this one. Splitting out the three prerequisites is what made this reviewable, and the ADR is going to save someone a lot of archaeology later. Nice work.
LOL |
|
Now we wait for the CI to be fixed... |
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
|
Vamos! |
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
ConfigBuilderinto a separate classConfigCmdAdapterin order to keep the core config builder in the runtime. This also provides a nice separation of concerns.I moved
K8sDriverLauncherintonf-cli-v1so that thenf-k8sis 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-k8son-demand as a plugin.I moved
PluginExecAwareintonextflowso 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