8000 Update PDC_KTH config for strict syntax by mahesh-panchal · Pull Request #913 · nf-core/configs · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Update PDC_KTH config for strict syntax #913

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Jun 16, 2025
Merged

Update PDC_KTH config for strict syntax #913

merged 4 commits into from
Jun 16, 2025

Conversation

mahesh-panchal
Copy link
Member
@mahesh-panchal mahesh-panchal commented Jun 12, 2025 •
edited
Loading

name: pdc_kth
about: Update pdc_kth in preparation for strict syntax.

  • Moves cluster name evaluation into parameter using a called closure.
  • Uses closure for singularity run Options.
  • clusterOptionsCreator moved into clusterOptions closure.
  • switch swapped to if because of strict syntax.
  • String concatenation changed to add to list then join.
  • Apply Nextflow code formatter.
  • Removes beforeScript because it's not called before images need to be pulled (i.e. user must do module load PDC apptainer themselves anyway).
  • Shorten nested if-else with ternary.

Please follow these steps before submitting your PR:

  • If your PR is a work in progress, include [WIP] in its title
  • Your PR targets the master branch
  • You've included links to relevant issues, if any

Steps for adding a new config profile:

  • Add your custom config file to the conf/ directory
  • Add your documentation file to the docs/ directory
  • Add your custom profile to the nfcore_custom.config file in the top-level directory
  • Add your profile name to the profile: scope in .github/workflows/main.yml
  • OPTIONAL: Add your custom profile path and GitHub user name to .github/CODEOWNERS (**/<custom-profile>** @<github-username>)

@mahesh-panchal mahesh-panchal requested a review from pontus June 12, 2025 08:06
@pontus
Copy link
Contributor
pontus commented Jun 12, 2025

Great, thanks.

Will need to look closer, but one immediate thing:

  • I think beforeScript is still needed because we need to have (a good) apptainer accessible in the job for calling. (I haven't checked how dardel is set up now, but I consider relying on environment inheritance from the submitting node for jobs is evil)

@mahesh-panchal
Copy link
Member Author 
* I think `beforeScript` is still needed because we need to have (a good) `apptainer` accessible in the job for calling. (I haven't checked how dardel is set up now, but I consider relying on environment inheritance from the submitting node for jobs is evil)

I'm not sure what you mean. Do you mean if they inadvertantly load ml PDC singularity instead?.
When pulling a container apptainer needs to be loaded already since it's performed before beforeScript gets called.

Although I just realised that the module environment propagates to the slurm environment. The beforeScript is run on the slurm node, while the apptainer pull is performed on the login node.

@pontus
Copy link
Contributor
pontus commented Jun 12, 2025

Yes, apptainer needs loading before running nextflow is started if it is to be able to pull images over https (although, if you have images already, you don't need it).

And environment from submit node might propagate to jobs. Partly. One should not rely on that.

And we need apptainer in the job and should thus load the module, thus beforeScript.

@pontus
Copy link
Contributor
pontus commented Jun 12, 2025

(So it doesn't matter what, if anything they have done before starting nextflow, we should still load the module in the job.)

@pontus
Copy link
Contributor
pontus commented Jun 13, 2025 via email

@mahesh-panchal mahesh-panchal marked this pull request as ready for review June 16, 2025 11:29
@mahesh-panchal
Copy link
Member Author

Added update to nf-schema validation too.

@pontus
Copy link
Contributor
pontus commented Jun 16, 2025

Feel free to merge whenever.

@mahesh-panchal mahesh-panchal merged commit 39ff2e8 into nf-core:master Jun 16, 2025
150 checks passed
@mahesh-panchal mahesh-panchal deleted the update_pdc_kth branch June 16, 2025 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pontus pontus pontus approved these changes

@maxulysse maxulysse maxulysse approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mahesh-panchal @pontus @maxulysse
0