Unify nf-lang config scopes with runtime classes #6271
Conversation
✅ Deploy Preview for nextflow-docs-staging canceled.
|
bentsherman
force-pushed
the
config-scopes
branch
3 times, most recently
from
824523f to
8ca91ca
Compare
| default boolean entrypointOverride() { | ||
| return ContainerHelper.entrypointOverride() | ||
| } |
There was a problem hiding this comment.
The entrypointOverride is a hidden config option that can be set for docker, k8s, and the other container config scopes.
I made it so that it can only be enabled by setting the NXF_CONTAINER_ENTRYPOINT_OVERRIDE environment variable, since that seems simpler than adding a config option for every container scope.
If we want to preserve this behavior, I suggest we document this env var and add a note to the 25.10 migration notes.
| if( containerConfig.writableInputMounts()==false ) | ||
| builder.params(readOnlyInputs: true) |
There was a problem hiding this comment.
It looks like some container scopes can specify writableInputMounts = false to mount folders into the container as read-only. There are tests for this option for apptainer, charliecloud, docker, podman, and singularity... so basically all of them.
If we want to keep this behavior, I suggest we add config option for each container scope where it's relevant. Or we could add an environment var.
| boolean getCleanup(boolean defValue=true) { | ||
| target.cleanup == null ? defValue : Boolean.valueOf( target.cleanup as String ) | ||
| defValue | ||
| } |
There was a problem hiding this comment.
There is a hidden option k8s.cleanup which is enabled by default, but can be disabled to not delete any pods. Normally only successful pods are deleted. Not sure if we still need this setting.
| String getUserName() { | ||
| target.userName ?: System.properties.get('user.name') | ||
| System.properties.get('user.name') | ||
| } |
There was a problem hiding this comment.
There is a hidden config option k8s.userName to control the user name, which is used to set the default launchDir. Since we can set launchDir directly I'm not sure that we need this option.
| String getNextflowImageName() { | ||
| final defImage = "nextflow/nextflow:${BuildInfo.version}" | ||
| return target.navigate('nextflow.image', defImage) | ||
| } | ||
|
|
||
| boolean getAutoMountHostPaths() { | ||
| Boolean.valueOf( target.autoMountHostPaths as String ) | ||
| return "nextflow/nextflow:${BuildInfo.version}" | ||
| } |
There was a problem hiding this comment.
There is a hidden option k8s.nextflow.image which controls the nextflow container image used by kuberun. This can already be controlled via CLI option so I don't think we need a config option for it.
|
|
||
| if( target.httpConnectTimeout ) | ||
| result.httpConnectTimeout = target.httpConnectTimeout as Duration | ||
| final result = clientDiscovery(context, namespace, serviceAccount) |
There was a problem hiding this comment.
There is a hidden scope k8s.client which can be used to define settings for the K8s client directly in Nextflow config. I think the best practice is to define these settings in .kube/config. If we want to keep this approach we should explicitly document it.
| void check(Map config, List<String> args) { | ||
| final store = LinStoreFactory.getOrCreate(LineageConfig.create(config), false) |
There was a problem hiding this comment.
@jorgee I refactored the lineage commands to not create a session since it was only used to create the LineageConfig and now isn't needed. However there are two failing unit tests:
LinCommandImplTest > should print error store is not found in diff FAILED
org.spockframework.runtime.SpockComparisonFailure at LinCommandImplTest.groovy:467
LinCommandImplTest > should print correct validate path FAILED
nextflow.exception.AbortOperationException at LinCommandImplTest.groovy:527
I wonder if it's a side effect of not creating the session? Do you know if the session is still needed (e.g. by the LID filesystem) for other purposes?
It would be nice to avoid creating a session since we aren't running a pipeline here, but if it requires deeper changes we could defer to a separate PR.
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
bentsherman
force-pushed
the
config-scopes
branch
from
8ca91ca to
9d5e6b8
Compare
This PR moves the config schema annotations into the runtime so that we don't have to update config options in both places.
Plugin config scopes are now defined in the plugins themselves. Core plugins are typically only loaded when they are actually used (i.e. the awsbatch executor), so when Nextflow validates config at runtime, it will skip validation of core plugin config if the core plugin isn't loaded. This allows the
lintcommand to work the same as before.I will update the language server to extract this metadata at build-time with a custom Gradle task. For third-party plugins we will need to publish this metadata to the plugin registry, but that can be done later.
I have removed several config options that were undocumented or appeared to be legacy options. We can preserve them if needed. See my comments below.
TODO: