Skip to content
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

Update cambridge.md #573

Merged
merged 8 commits into from
Oct 30, 2023
Merged

Update cambridge.md #573

merged 8 commits into from
Oct 30, 2023

Conversation

EmelineFavreau
Copy link
Contributor

The instructions include the latest changes in config file: https://github.com/nf-core/configs/blob/master/conf/cambridge.config


name: Updated Config documentation for Cambridge HPC
about: An existing cluster config

This allows the user to understand how to use the options partition, project and cacheDir.

The instructions include the latest changes () in config file: https://github.com/nf-core/configs/blob/master/conf/cambridge.config
@EmelineFavreau EmelineFavreau requested a review from jfy133 October 18, 2023 10:31
@jfy133
Copy link
Member

jfy133 commented Oct 18, 2023

@nf-core-bot fix linting

docs/cambridge.md Show resolved Hide resolved
docs/cambridge.md Show resolved Hide resolved
tar xvfz jdk-20_linux-x64_bin.tar.gz
# add these lines to .bashrc
export JAVA_HOME=/home/ef479/jdk-20.0.1
export PATH=/home/ef479/jdk-20.0.1/bin:$PATH
Copy link
Member

Choose a reason for hiding this comment

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

This also seems dangerous, what if someone needs to use a different tool that isn't v20 compatible? Another reason why I suggest writing Conde instructions above (and install mxf into a separate env)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated this.


# Launch the nf-core pipeline for a test database
# with the Cambridge profile
nextflow run nf-core/sarek -profile test,cambridge.config --partition "cclake" --project "NAME-SL3-CPU" --cacheDir "/home/<username>/rds/hpc-work/path/to/cache/dir" --outdir nf-sarek-test
Copy link
Member

Choose a reason for hiding this comment

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

CacheDir isn't a parameter in any nf-core pipeline afaik, this could break input validatio - particularly if someone uses the config in a non nf-core pipeline that does stricter testing!!!.

You sound instead tell users to use the NXF_SINGULARITY_CACHEDIR bash env variable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this. I changed the documentation here fe1d2ec.

alternative way of getting nextflow using conda
Conda install is suggested first as a preferred method.
Singularity cache is now a bash variable.
This removes the params config that could have broken input validation.
@EmelineFavreau EmelineFavreau requested a review from jfy133 October 25, 2023 10:04
removing cachedir in params
- singularity is now an option to load as module, because it should be loaded on login by default.
- the cache directory contains more explanation as to why it can be created
@jfy133
Copy link
Member

jfy133 commented Oct 27, 2023

@nf-core-bot fix linting

(Also I've not forgotten @EmelineFavreau ! Getting to this this afternoon!)

Copy link
Member

@jfy133 jfy133 left a comment

Choose a reason for hiding this comment

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

I'm still unsure about the non-pipeline Params, but I'm assuming you're testing this with no issue for the time being so I approve this for now. However I would consider coming up with a different method in the future as it may conflict with some pipelines (particularly non-nf-core ones)

@@ -5,7 +5,6 @@ params {
config_profile_url = "https://docs.hpc.cam.ac.uk/hpc"
partition = null
project = null
Copy link
Member

Choose a reason for hiding this comment

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

This and partition should also be removed as well theoretically

```
# Launch the nf-core pipeline for a test database
# with the Cambridge profile
nextflow run nf-core/sarek -profile test,cambridge.config --partition "cclake" --project "NAME-SL3-CPU" --outdir nf-sarek-test
Copy link
Member

Choose a reason for hiding this comment

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

Is partition and project in sarek?

Copy link
Member

Choose a reason for hiding this comment

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

It might be ok to keep, but you may end up with validation errors as they aren't pipeline specific parameters. It would make make more sense to have them set as environmental variables and insert them via that way

@EmelineFavreau EmelineFavreau merged commit 64aabaa into master Oct 30, 2023
209 checks passed
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