Skip to content

Conversation

bentsherman
Copy link
Member

@bentsherman bentsherman commented Apr 14, 2025

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

This comment was marked as off-topic.

@bentsherman
Copy link
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
Member

oh mamma mia 😆

@bentsherman
Copy link
Member Author

Your k8s plugin inspired me 😄

@pditommaso
Copy link
Member

I feared that 😆

@bentsherman
Copy link
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
Member

I'll review post 25.04

@bentsherman
Copy link
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
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.

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'
Copy link
Member

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

Copy link
Member Author

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

@pditommaso
Copy link
Member

Did I break anything? I see some tests failing. Also trying locally I get

» ./launch.sh run hello

 N E X T F L O W   ~  version 25.06.0-edge

NOTE: Your local project version looks outdated - a different revision is available in the remote repository [2ce0b0e294]
Launching `https://github.com/nextflow-io/hello` [backstabbing_ride] DSL2 - revision: afff16a9b4 [master]

ERROR ~ No such variable: session

@bentsherman
Copy link
Member Author

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

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

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>
@bentsherman bentsherman added this to the 25.10 milestone Jul 14, 2025
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.

2 participants