Skip to content

Separate CLI from runtime - #5971

Merged
pditommaso merged 6 commits into
masterfrom
nf-runtime
Aug 6, 2026
Merged

Separate CLI from runtime#5971
pditommaso merged 6 commits into
masterfrom
nf-runtime

Conversation

@bentsherman

@bentsherman bentsherman commented Apr 14, 2025

Copy link
Copy Markdown
Member

This PR splits the nextflow module into two modules:

  • nf-cli-v1: contains only the Launcher and CLI classes
  • nextflow: contains everything (i.e. the "runtime")

Benefits:

  • Improves build time, editing CLI code doesn't rebuild the entire project
  • Removes some circular dependencies, making the code easier to read/understand
  • Library consumers like platform or plugins can just depend on the runtime and not the CLI
  • Makes it easier to implement a CLI v2 in the future

Notes:

  • I moved the CLI bits of ConfigBuilder into a separate class ConfigCmdAdapter in order to keep the core config builder in the runtime. This also provides a nice separation of concerns.

  • I moved K8sDriverLauncher into nf-cli-v1 so that the nf-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 load nf-k8s on-demand as a plugin.

  • I moved PluginExecAware into nextflow so that plugins can declare CLI commands without depending on nf-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

@netlify

This comment was marked as off-topic.

@bentsherman

Copy link
Copy Markdown
Member Author

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.

@pditommaso

Copy link
Copy Markdown
Member

oh mamma mia 😆

@bentsherman

Copy link
Copy Markdown
Member Author

Your k8s plugin inspired me 😄

@pditommaso

Copy link
Copy Markdown
Member

I feared that 😆

@bentsherman

Copy link
Copy Markdown
Member Author

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

@pditommaso

Copy link
Copy Markdown
Member

I'll review post 25.04

@bentsherman

Copy link
Copy Markdown
Member Author

Refactored so that the CLI code is in a new module nf-cli-v1. Made the PR considerably smaller

Note the following plugins depend explicitly on the CLI code because they add CLI commands:

  • nf-console
  • nf-k8s
  • nf-tower
  • nf-wave

Might be possible to remove this dependency by moving some interfaces into the core runtime, but not a big deal either way

@bentsherman
bentsherman force-pushed the nf-runtime branch 2 times, most recently from ac8c183 to bf822ef Compare April 19, 2025 00:33
@pditommaso
pditommaso force-pushed the master branch 3 times, most recently from b4b321e to 069653d Compare June 4, 2025 18:54
@bentsherman
bentsherman force-pushed the nf-runtime branch 4 times, most recently from 5cc1731 to fd1408d Compare June 18, 2025 21:27
@bentsherman
bentsherman removed the request for review from tom-seqera June 18, 2025 21:29
@bentsherman

Copy link
Copy Markdown
Member Author

I refactored the nf-lineage module to depend on nextflow and be required by nf-cli-v1. This removes the need for the LinCommand interface as the lineage command can simply use LinCommandImpl directly.

Couldn't quite do this for nf-k8s because the K8sDriverLauncher uses CmdRun, so that's a pretty hard dependency on the CLI v1. We could make nf-k8s a module instead of a plugin, but then it would be included at build-time instead of runtime.

Comment thread modules/nextflow/build.gradle Outdated
@pditommaso

This comment was marked as resolved.

@bentsherman

This comment was marked as resolved.

@bentsherman
bentsherman force-pushed the nf-runtime branch 2 times, most recently from 498fd1e to 0faaf13 Compare July 8, 2025 20:21
@bentsherman

This comment was marked as resolved.

@bentsherman
bentsherman marked this pull request as draft July 21, 2026 18:30
@bentsherman

Copy link
Copy Markdown
Member Author

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

@bentsherman
bentsherman requested review from jorgee and removed request for jorgee July 27, 2026 14:39
@bentsherman
bentsherman force-pushed the nf-runtime branch 2 times, most recently from c394425 to b20d002 Compare July 27, 2026 15:13
@bentsherman
bentsherman marked this pull request as ready for review July 27, 2026 15:14
@pditommaso

Copy link
Copy Markdown
Member

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. ConfigCmdAdapter showing up as a pure 0-line rename is exactly what I was hoping for, and having the three prerequisites land separately means this one can be cherry-picked or reverted on its own.

I went back through the spin-outs and they all check out:

The ADR is a genuinely good addition. Calling out the WaveCmdEntry regression yourself rather than letting it be discovered later is the right instinct.

A few new things I picked up on this pass, none of them fundamental:

Worth fixing before merge

packing.gradleant.copy"cp $source $target".execute(). Groovy's String.execute() doesn't wait for the process and ignores its exit status, and this sits in packOne on the release path — a downstream task could pick up a partial jar with nothing surfacing. I think the change was only needed to rename the artifact, so ant.copy(file: source, tofile: ..., overwrite: true) or archiveBaseName = 'nextflow' in the shadowJar block would do it safely.

nf-k8s now ships inside the distribution fat jar. Dropping the explicit configurations = [...] from shadowJar means it falls back to runtimeClasspath, which now includes the api(project(':plugins:nf-k8s')) dep. I probed it on the branch:

SHADOW_CONFIGS: [configuration ':nf-cli-v1:runtimeClasspath']
K8S_RELATED_IN_FATJAR: [nf-k8s-1.5.5.jar, bcpkix-jdk18on-1.84.jar, bcprov-jdk18on-1.84.jar]

Since nf-k8s is still distributed as a plugin too (plugins-info.txt: nf-k8s@1.5.5), the same classes end up in both the app and plugin classloaders, and nextflow.k8s becomes a split package. The ADR accepts the build-time dependency, which is fair, but this is the part that could actually bite — plugin version pinning silently becoming a no-op, or cross-classloader ClassCastException. Keeping configurations explicit and excluding nf-k8s would contain it.

To be clear though, dissolving the lineageImplementation hack is the cleanest win in the whole PR — that workaround has annoyed me for a long time.

Test heap for the nextflow module. The test { minHeapSize/maxHeapSize } block moved to nf-cli-v1 rather than being duplicated, so :nextflow:test now resolves to null for both while the root build sets no default. The largest suite in the repo drops from an explicit 4 GB, and the module with a fraction of the tests inherits it. CI passes, so it fits today — looks unintentional rather than harmful. Probably belongs in the root subprojects block so it can't drift again.

CmdKubeRun no longer calls Plugins.init() / Plugins.start('nf-k8s'). The old path initialized the plugin system before resolving the extension; the new one constructs K8sDriverLauncher directly. Whether that matters depends on the k8s config scope, which is registered via the nextflow.k8s.K8sConfig extension point. Nothing in CI exercises kuberun, so this probably wants a manual smoke test before merge.

PluginExecAware breaks third-party plugins twice — package move and signature change. The ADR covers the signature; worth a changelog note covering both.

Nits

  • LoggerOptions has no class javadoc and no @author, unlike the rest of the codebase. With seven positional params and three same-typed booleans, the javadoc earns its keep here — callers have only parameter order to go on. Also slightly ironic that we landed HubOptions away from a record in Refactor HubOptions from a trait into a record #7372 and introduced one here.
  • AuthCommand.groovy and LaunchCommand.groovy headers say 2013-2025.
  • LaunchOptions could use its own file rather than riding along in LaunchCommand.groovy.
  • -XDignore.symbol.file went to nf-cli-v1 (one internal-API import) while the four files that motivate it — Session, AbortSignalException, ThreadPoolHelper, LocalPollingMonitor — stayed in nextflow. It's inert either way since both modules compile through groovyc, so this is cosmetic.
  • Stale comment in packing.gradle: "default cfg = runtime + httpfs + lineage", but lineage now arrives transitively.
  • A handful of api('g:a:v')api 'g:a:v' reformats snuck back in.

CI

The four failures are all 90-minute timeouts on test_aws/test_wave, hanging in "running rnaseq-nf with Fusion on AWS Batch" after job submission — that's infrastructure, not your change. Everything this PR touches passed. But the run is from 7/27 and master has moved 16 commits since, so it needs a re-run before merge regardless.

Nothing here is structural. Happy to approve once the packaging and heap bits are sorted.

@bentsherman

Copy link
Copy Markdown
Member Author

No problem. This PR deserves plenty of scrutiny.

Updates:

  • restored the ant.copy with tofile:
  • moved test heap settings to top-level build.gradle -- this seemed sensible to me since test is already configured there
  • added note about PluginExecAware package change to ADR -- I will document this as a breaking change in the 26.10 migration notes
  • refactored LoggerOptions as a @Canonical class (matching existing codebase pattern) and added a brief javadoc
  • moved LaunchOptions to its own file
  • verified that -XDignore.symbol.file is inert, removed it
  • fixed other nits you mentioned

Regarding your concerns about nf-k8s -- as far as I can tell, the current refactor is the cleanest way to support kuberun in CLI v1 while keeping nf-k8s as a plugin. The only other option I see is to change nf-k8s back to a core module like nf-lineage.

I have updated the ADR with the split-package consequence. I will also perform some manual tests to make sure everything still works

@bentsherman

Copy link
Copy Markdown
Member Author

Tested kuberun locally. Found a regression from #7372 and patched it here. With that fix, kuberun is able to use nf-k8s classes without any issues.

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 k8s config scope being registered twice. No failure, config validates the same as before, and there is no reason to do this in the first place.

@pditommaso

Copy link
Copy Markdown
Member

Went through the updates and re-validated the parts that needed evidence. Everything from the last round is addressed — ant.copy with tofile:, heap settings consolidated in the root build.gradle, LoggerOptions as a @Canonical class with a javadoc, LaunchOptions in its own file, -XDignore.symbol.file gone, headers and gradle reformats cleaned up. All confirmed on d080c32a4.

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:

Extension executors providers=[K8sExecutor, K8sExecutor]
Replacing executor K8sExecutor with class nextflow.k8s.K8sExecutor
WINNER_CL=org.pf4j.PluginClassLoader@59bbb974

pf4j enumerates classpath extensions before plugin extensions, so the plugin copy always wins. Pinning holds and there's no cross-classloader hazard in practice.

PluginExecAware — consider restoring the old package name

I built :nf-cli-v1:shadowJar from this branch and ran it against the published nf-tower@1.29.1 straight from the registry:

INFO  org.pf4j.AbstractPluginManager - Start plugin 'nf-tower@1.29.1'
ERROR nextflow.cli.Launcher - @unknown
java.lang.NoClassDefFoundError: nextflow/cli/PluginExecAware
Caused by: java.lang.ClassNotFoundException: nextflow.cli.PluginExecAware
	at org.pf4j.PluginClassLoader.loadClass(PluginClassLoader.java:169)

TowerPlugin and WavePlugin implement the interface on the plugin class itself, so it resolves eagerly at plugin start and takes down the whole command — not just nextflow plugin exec. I scanned the published jar and PluginExecAware + PluginAbstractExec are the only two nextflow.cli.* classes it can no longer find; Launcher, CmdAuth and CmdLaunch all still resolve because nf-cli-v1 kept the package.

So I tried moving both back to nextflow.cli — two renames plus five one-line imports, 8 lines total, the classes stay physically in the nextflow runtime module. Rebuilt, reran the same command with the same published plugin: it completes normally.

The package name isn't what buys the decoupling — the module is. And the runtime already hosts nextflow.cli.AuthCommand, LaunchCommand and LaunchOptions, so nextflow.cli is split across the two modules either way; keeping these two there is arguably more consistent than carving out nextflow.plugin.cli.

The signature change can stay as-is. Old implementors load fine and only fail if someone actually invokes nextflow plugin <id>:<cmd> — and since PluginAbstractExec is a trait, old subclasses carry baked-in $Trait$Helper calls with the old descriptor, so it's a NoSuchMethodError at invocation rather than at load. Narrow and loud, instead of total.

To be clear, I'm not worried about the plugin VERSION files — those get bumped at release time. The point is that the failure mode for third-party plugins you can't rebuild goes from "nothing works" to "one command is unavailable".

Residual issue: AuthCommand / LaunchCommand nested → top-level

With the crash out of the way, this showed up underneath it:

ERROR org.pf4j.AbstractExtensionFinder - nextflow/cli/CmdLaunch$LaunchCommand
ERROR org.pf4j.AbstractExtensionFinder - nextflow/cli/CmdAuth$AuthCommand

Promoting CmdAuth.AuthCommand and CmdLaunch.LaunchCommand out of their enclosing classes breaks the same published plugin. pf4j logs and skips, so it's non-fatal, but nf-tower's launch and auth extensions silently disappear. Keeping the nested names as interface AuthCommand extends nextflow.cli.AuthCommand would shim it — old implementors would still satisfy the new type. Your call whether that's worth it.

ADR needs two corrections

The k8s scope warning isn't limited to explicitly declaring the plugin. Plain process.executor = 'k8s' auto-loads nf-k8s via PluginsFacade:520 and both copies register:

WARN nextflow.config.ConfigValidator - Plugin config scope `nextflow.k8s.K8sConfig` conflicts with existing scope: `k8s`

That's every k8s run. The mechanism is the shadow AppendingTransformer: nextflow.k8s.K8sConfig and K8sExecutor end up at lines 39–40 of the fat jar's META-INF/extensions.idx, so they register as system extensions on every run, k8s or not. If you'd rather not see it in the wild, deduping in ConfigValidator.loadPluginScopes() when the duplicate is the same class name is a couple of lines and covers future split packages too.

Second, the bundled copy silently substitutes for the plugin. With NXF_PLUGINS_DEFAULT=false, executor = 'k8s' still resolves from the app classloader — previously it wouldn't have. Benign, but worth a line in the consequences.

Nits

  • modules/nf-lineage/src/resources/META-INF/extensions.idx still says 2013-2025.
  • Launcher.loggerOptions() builds LoggerOptions with seven positional args including three booleans. @Canonical gives you the map constructor for free — new LoggerOptions(ansiLog: …, background: …) would make a transposition impossible.

CI

Fresh run on d080c32a4, so my "needs a re-run" note is resolved. Unit tests green on 17 and 25; test_integration, test_parser_v2, test_azure, test_google, test_docs all pass. The four test_aws/test_wave 90-minute timeouts have the same infrastructure signature as before.

Nothing structural left from my side — the split reads clean. Whatever you decide on the package, worth updating the ADR wording before merge.

@bentsherman

Copy link
Copy Markdown
Member Author
  • Restored PluginExecAware and PluginAbstractExec to their original packages. good catch, agree this is the right move

  • I did not add anything for AuthCommand / LaunchCommand. There is never any reason to use an older nf-tower with a newer Nextflow version since a nf-tower version is pinned to each release. Documented in the ADR

  • Documented the double-loading of nf-k8s via process.executor = 'k8s'. Updated ConfigValidator to suppress the duplicate-registration warning for nf-k8s

  • Addressed other nits

@pditommaso pditommaso left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.1 pulled from the registry. Plugin starts, command exits 0. No nextflow.plugin.cli references left anywhere, docs import updated too.
  • The ConfigValidator fix 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 zero conflicts with existing scope lines on process.executor = 'k8s', with k8s.namespace still validating.
  • On the LoggerOptions named args I checked propagation rather than just the compile, since a silent map-constructor mismatch under @CompileStatic would have been invisible: -log writes to the given path and -trace nextflow.plugin yields 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.

@pditommaso

Copy link
Copy Markdown
Member

the ADR is going to save someone a lot of archaeology later

LOL

@bentsherman

Copy link
Copy Markdown
Member Author

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>
@pditommaso

Copy link
Copy Markdown
Member

Vamos!

@pditommaso
pditommaso merged commit 1dc8cf6 into master Aug 6, 2026
24 checks passed
@pditommaso
pditommaso deleted the nf-runtime branch August 6, 2026 07:35
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.

3 participants