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

Adding a pipeline specific config for a specific system #85

Merged
merged 34 commits into from
Nov 27, 2019

Conversation

maxulysse
Copy link
Member

@maxulysse maxulysse commented Nov 20, 2019

@maxulysse maxulysse added the enhancement New feature or request label Nov 20, 2019
@maxulysse maxulysse requested a review from a team November 20, 2019 09:22
@maxulysse maxulysse self-assigned this Nov 20, 2019
Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

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

This is very nice! 💖

  • Needs some documentation on the main README.md about how this works
  • I think I would prefer pipelines/sarek.config instead of pipeline/nfcore_custom_sarek.config (nfcore_custom_ is kind of a duplicate of the repo already)..


params {
config_profile_contact = 'Maxime Garcia (@MaxUlysse)'
config_profile_description = 'sarek MUNIN profile provided by nf-core/configs.'
Copy link
Member

Choose a reason for hiding this comment

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

Presumably this overwrites the param set in the main config?

config_profile_description = 'MUNIN profile provided by nf-core/configs.'

I guess this is fine.. Maybe not necessary though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the second config file overwrite all the params from the first one.
It would be better to append actually in that case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I do think it's good to keep that, that way you know that the main institute profile has been overwritten by a pipeline/institute config

Copy link
Member

Choose a reason for hiding this comment

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

Not necessarily completely overwritten though. Most of the time it will be adding additional stuff I imagine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Existing params are overwritten

Copy link
Member Author

Choose a reason for hiding this comment

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

But yeah, I do think it's good to know that this is more than the nf-core/configs classic institutional profile

* -------------------------------------------------
* nfcore custom profile Nextflow config file
* -------------------------------------------------
* Config options for all custom environments.
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to update this a little to explain the difference to the main config profiles and mention that it's not necessary for all institutes / pipelines.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm mainly playing now, but will make docs for that.
so that we can do that for UPPMAX as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Added some docs.
Please tell me what do you think about it.

pipeline/sarek.config Outdated Show resolved Hide resolved
@maxulysse
Copy link
Member Author

sorry for the commit mess in the end, I'll try rebasing

@maxulysse
Copy link
Member Author

maxulysse commented Nov 20, 2019

Both following commands worked as expected on munin.

nextflow run MaxUlysse/sarek -r sentieon -profile test,munin --sentieon /
--custom_config_base 'https://raw.githubusercontent.com/MaxUlysse/nf-core_configs/sarek'
nextflow run MaxUlysse/sarek -r sentieon -profile test,munin /
--custom_config_base 'https://raw.githubusercontent.com/MaxUlysse/nf-core_configs/sarek'

All new params were used, and the sentieon specific module was only loaded (and container set to '' ) for all processes with the sentieon label when the params.sentieon was specified.

@maxulysse maxulysse requested review from szilvajuhos and a team November 20, 2019 16:51
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.

Some documentation suggestions for clarity.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
conf/munin.config Outdated Show resolved Hide resolved
conf/munin.config Outdated Show resolved Hide resolved
Copy link

@szilvajuhos szilvajuhos left a comment

Choose a reason for hiding this comment

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

Cool

Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

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

Code looks great! Added some suggestions for rephrasing / additions in the documentation..

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
conf/munin.config Outdated Show resolved Hide resolved
conf/pipeline/sarek/munin.config Outdated Show resolved Hide resolved
conf/pipeline/sarek/munin.config Show resolved Hide resolved
docs/pipeline/sarek/munin.md Show resolved Hide resolved
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.

Documentation is much improved @maxulysse ! Couple more clarifying edits.

Let me know if you want a beta-tester of the system - I have a test case to apply it on but would rather wait for a request to do so.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
@maxulysse
Copy link
Member Author

If you're OK with my latest changes, then we can merge it, and I'll help you apply that to eager ;-)

@jfy133
Copy link
Member

jfy133 commented Nov 27, 2019

Good with me (docs wise, code I'll leave to someone else ;))

@maxulysse maxulysse merged commit 64ff62d into nf-core:master Nov 27, 2019
@maxulysse maxulysse deleted the sarek branch November 27, 2019 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants