Skip to content

Make vsc_calcua strict syntax compatible#1016

Open
nvnieuwk wants to merge 5 commits intonf-core:masterfrom
nvnieuwk:strict/vsc_calcua
Open

Make vsc_calcua strict syntax compatible#1016
nvnieuwk wants to merge 5 commits intonf-core:masterfrom
nvnieuwk:strict/vsc_calcua

Conversation

@nvnieuwk
Copy link
Contributor

This PRs converts the config to the strict syntax.

@pmoris would you mind testing this config on the cluster it is designed for? Please run a test with strict syntax enabled (NXF_SYNTAX_PARSER=v2) and with it disabled (NXF_SYNTAX_PARSER=v1) to ensure backwards compatibility.

Let me know if something's broken 😁

@pmoris
Copy link
Contributor

pmoris commented Feb 18, 2026

I'll give it a go (soonTM). Cheers for the update!

@pontus pontus requested a review from pmoris March 13, 2026 06:35
@pmoris
Copy link
Contributor

pmoris commented Mar 13, 2026

I haven't gotten around to testing or reviewing the changes yet, but I'll allocate some time to do so next week. Thanks for the help in any case!

@pmoris
Copy link
Contributor

pmoris commented Mar 23, 2026

OK! I've tested the new config and had to make the following changes:

  • Changes param "functions" into dynamic variables.
  • Fixes regression: dynamic allocation of resources should be used in single node
    execution mode instead of slurm scheduling mode.
  • Use explicit int/str types for dynamic resource allocation to avoid weird behaviour.

After these adjustments, everything seems to work when using either NXF_VER=26.02.0-edge NXF_SYNTAX_PARSER=v2 nextflow run nf-core/demo or NXF_VER=26.02.0-edge NXF_SYNTAX_PARSER=v1 nextflow run nf-core/demo, both in the slurm scheduling and the single node execution modes.

What I'm still unsure about:

https://nextflow.slack.com/archives/C02TPSL19UJ/p1774277999098809

I came across a weird behaviour when comparing v1 and v2 strict syntax, where it seems that using the elvis operator on a mix of strings and integers somehow converts str(4) into int(52)

def node_max_cpu = 28

def int max_cpus = node_max_cpu

def slurm_task = '4'
def slurm_node = '4'

max_cpus = slurm_task ?: slurm_node ?: node_max_cpu

println "max_cpus = ${max_cpus} (type: ${max_cpus.class.name})"


└─▶ NXF_VER=26.02.0-edge NXF_SYNTAX_PARSER=v1 nextflow run test.nf
Nextflow 26.03.0-edge is available - Please consider updating your version to it

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

Launching `test.nf` [wise_solvay] revision: 265d31ac18

max_cpus = 52 (type: java.lang.Integer)


└─▶ NXF_VER=26.02.0-edge NXF_SYNTAX_PARSER=v2 nextflow run test.nf
Nextflow 26.03.0-edge is available - Please consider updating your version to it

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

Launching `test.nf` [desperate_cray] revision: 265d31ac18

max_cpus = 4 (type: java.lang.String)

@pmoris pmoris self-assigned this Mar 23, 2026
@nvnieuwk
Copy link
Contributor Author

You also have a third merge conflict at the end of the config, could you resolve this first?

@pmoris
Copy link
Contributor

pmoris commented Mar 24, 2026

Ack. That's what I get for not double-checking. Things went awry when I switched between two machines and juggled a git stash.

Added a fixup. You can do a rebase --autosquash before eventually merging to clean up the commit history a bit.

@nvnieuwk
Copy link
Contributor Author

Looks good to me, feel free to approve and merge if you think this is ready

@pmoris
Copy link
Contributor

pmoris commented Mar 24, 2026

There's a bunch of exit statements in the config that the automatic tests seem to have problems with. I guess I could change them into warnings instead?

@nvnieuwk
Copy link
Contributor Author

Yeah that might be for the best

@pmoris
Copy link
Contributor

pmoris commented Mar 24, 2026

To everyone I somehow tagged => ignore this. GH keeps messing with my attempts to rebase/squash instead of merge...

@pmoris pmoris removed request for jen-reeve and sguizard March 24, 2026 15:31
@pmoris pmoris force-pushed the strict/vsc_calcua branch 2 times, most recently from de59fd3 to fc42f62 Compare March 24, 2026 15:40
nvnieuwk and others added 4 commits March 24, 2026 16:42
…e comments

- Changes param "functions" into dynamic variables.
- Fixes regression: dynamic allocation of resources should be used in single node
execution mode instead of slurm scheduling mode.
- Use explicit int/str types for dynamic resource allocation to avoid weird behaviour.

In depth explanation:

Functions are no longer allowed in the new strict syntax mode, see:
https://nf-co.re/docs/tutorials/migrate_to_strict_syntax/config_strict_syntax#functions).

However, the proposed param "functions" caused execution errors:

`groovy.lang.MissingMethodException: No signature of method: java.util.LinkedHashMap.vsc_calcua_get_allocated_mem_func() is applicable for argument types: (Integer)`

This commit instead tries to mimic the old functionality using dynamic variables
as described here:
https://nf-co.re/docs/tutorials/migrate_to_strict_syntax/config_strict_syntax#dynamic-variables

Note: care should be taken when mixing integers and strings in ternary / elvis operators
when using explicitly typed variables. E.g. in this case a weird mix of `def int max_cpus` with
untyped strings for the slurm cpu count seemed to convert the string '4' into the int 52.
The system.exit(1) calls seemed to cause problems with
the automatic testing, so these were replaced by warnings.
This should make it easier to run two-step pipelines and troubleshoot.
@pmoris pmoris force-pushed the strict/vsc_calcua branch from fc42f62 to 6bcbc1d Compare March 24, 2026 15:42
Copy link
Contributor

@pmoris pmoris left a comment

Choose a reason for hiding this comment

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

Seems to work on CalcUA, but some of the tests keep failing even after I removed the system.exit() calls. ! Is it OK if I just ignore them? I think the issue is with other configs, rather than changes we made to this one.

@nvnieuwk
Copy link
Contributor Author

The test for your config works so feel free to merge :)

@nvnieuwk
Copy link
Contributor Author

@nf-core-bot fix linting

@pmoris
Copy link
Contributor

pmoris commented Mar 24, 2026

The test for your config works so feel free to merge :)

Merging is blocked due to failing merge requirements => isn't that caused by the failing tests?

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