Skip to content

Update nci_gadi.config #872

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.

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

kisarur
Copy link

@kisarur kisarur commented Mar 25, 2025


name: Update NCI Gadi Config
about: A new update to NCI Gadi cluster config

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 custom profile to the README.md 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>)

@pontus
Copy link
Contributor

pontus commented Mar 25, 2025

Since this seems to change behaviour, don't you want documentation updates as well?

Similarly you might consider defaulting to previous behaviours if these new parameters are not provided.

I also think you might want to work around validator warnings for the schema.

@jfy133
Copy link
Member

jfy133 commented Mar 25, 2025

I agree, we also don't want to encourage undocumented 'pipeline params' (as these do not come from pipelines) which is why we promote the usage of the env variables currently

@kisarur
Copy link
Author

kisarur commented Mar 25, 2025

Thanks, @pontus and @jfy133! I can definitely update the documentation.

To make the config more generalised, I was trying to use user-defined parameters for project (used for billing on our cluster, which can be different from the default project obtained through the PROJECT env variable) and storage (used to specify which storage mounts are required for the PBS jobs spawned by the pipeline. Note that these storage mounts may come from different projects than the billing project and can also originate from multiple projects). In the previous version of the config, both project and storage were set based on the same PROJECT environment variable, making it a less flexible approach.

Regarding the default behavior: if the user does not set these values using --project or --storage, they default to use the PROJECT env variable (this ensures backward compatibility).

BTW, to avoid potential conflicts with parameters that users might define in their pipeline, I can rename project and storage to something like nci_project and nci_storage if needed.

@pontus
Copy link
Contributor

pontus commented Mar 26, 2025

It's good if they default to the old behaviour. I still think it would make sense to update the documentation but it makes it somewhat less important I guess. But out of curiosity - how does that (or it all) work?

I see the docs say "The version of Nextflow installed on Gadi has been modified to make it easier to specify resource options for jobs submitted to the cluster." - I guess that means somehow picking up and using storage and project from the process.

Does that also do something to make nf-validator/nf-schema not complain about these params?

@kisarur
Copy link
Author

kisarur commented Mar 28, 2025

Regarding defaulting to the old behavior: I assume that users of the old version would not have params.project and params.storage set to anything, as the old version did not effectively modify process.project (which was always set to the env variable PROJECT) or process.storage (which was set to scratch/PROJECT using params.project which defaulted to the env variable PROJECT). Now, if an old user adopts the new config (without setting params.project to anything, as expected), params.project will default to the env variable PROJECT (Line 6), and process.project (Line 29) will be assigned this default value (reproducing the old behavior). Similarly, if the user doesn't specify storage via params.storage, params.storage will be assigned gdata/PROJECT+scratch/PROJECT (Line 7), which is then used to set process.storage (Line 30). The gdata/PROJECT storage is added as an additional mount by default, as it is commonly used by users (scratch/PROJECT will already be present, ensuring consistency for old users).

Yes, the modified version of Nextflow installed on Gadi picks up process.project, process.storage (not params.project, params.storage) to set the project for billing and storage mounts, respectively. BTW, I don't think this modification does something to make nf-validator/nf-schema not complain about params.project, params.storage. Would you recommend any way to handle this in the config?

@pontus
Copy link
Contributor

pontus commented Mar 28, 2025

Ah, thanks, it seems I slipped when thinking.

As for validation, I haven't tested myself lately, but https://github.com/nf-core/configs/blob/master/conf/uppmax.config#L9-L10 shows what was done to not get complaints some time ago (both were needed because of different versions included with different pipelines if I recall correctly).

@jfy133
Copy link
Member

jfy133 commented Jun 18, 2025

@kisarur do you plan to finish this PR?

@georgiesamaha georgiesamaha mentioned this pull request Jun 25, 2025
8 tasks
georgiesamaha added a commit to Sydney-Informatics-Hub/configs that referenced this pull request Jun 25, 2025
@kisarur
Copy link
Author

kisarur commented Jun 27, 2025

@jfy133 sorry for the late response — I got caught up with some other projects. I can include the suggested workaround to ignore the validator warnings if it's mandatory.
@pontus thanks for approving the changes.

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