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

Add script recommendations #2888

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Add script recommendations #2888

wants to merge 15 commits into from

Conversation

pinin4fjords
Copy link
Member

@pinin4fjords pinin4fjords commented Dec 6, 2024

Propose some lines about module scripts.

@netlify /docs/guidelines/components/modules

@pinin4fjords pinin4fjords marked this pull request as draft December 6, 2024 16:49
@pinin4fjords pinin4fjords requested a review from jfy133 December 6, 2024 16:49
Copy link

netlify bot commented Dec 6, 2024

Deploy Preview for nf-core-main-site ready!

Name Link
🔨 Latest commit c438115
🔍 Latest deploy log https://app.netlify.com/sites/nf-core-main-site/deploys/6759db8f6566850008f6b011
😎 Deploy Preview https://deploy-preview-2888--nf-core-main-site.netlify.app/docs/guidelines/components/modules
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Dec 6, 2024

Deploy Preview for nf-core-docs ready!

Name Link
🔨 Latest commit c438115
🔍 Latest deploy log https://app.netlify.com/sites/nf-core-docs/deploys/6759db8fdbef930008982d13
😎 Deploy Preview https://deploy-preview-2888--nf-core-docs.netlify.app/docs/guidelines/components/modules
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@pinin4fjords
Copy link
Member Author

@nf-core-bot fix linting

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.

Ugh, escaping in suggestions is hard... hope you undersatnd it...

I also decided to be more strict, and require a stadnardised tempalte file naming structure (Which would require an update to deseq2/differential, let me know what you think...

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.

A few further tweaks


For example, for R you can refer to the `deseq2/differential` module [here](https://github.com/nf-core/modules/blob/4c2d06a5e79abf08ba7f04c58e39c7dad75f094d/modules/nf-core/deseq2/differential/templates/deseq_de.R#L509-L534).

#### Script template stub
Copy link
Member

@jfy133 jfy133 Dec 9, 2024

Choose a reason for hiding this comment

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

Suggested change
#### Script template stub
#### Module template stub

@pinin4fjords
Copy link
Member Author

@nf-core-bot fix linting

@jfy133 jfy133 marked this pull request as ready for review December 9, 2024 11:26
@SPPearce
Copy link
Contributor

Can we reference the reason why not to use module binaries?
Personally I don't really like the interface to template, and I hate to see R scripts written with base R and the horribly clunky code to allow arguments in that way. I would suggest that we encourage the use of a seqera container with an appropriate interface package (e.g. argparse).
We could do with more explanation as to how to actualy use template, because I don't think we have that here (and the Seqera docs are almost non-existent on the topic).

I much prefer the use of in-line scripts where possible, what is the justification behind saying it can only be 20 lines?

@SPPearce
Copy link
Contributor

Oh, and some examples pointing to other modules rather than deseq2/differential repeatedly would be good.

@christopher-hakkaart
Copy link
Member

Can we reference the reason why not to use module binaries? Personally I don't really like the interface to template, and I hate to see R scripts written with base R and the horribly clunky code to allow arguments in that way. I would suggest that we encourage the use of a seqera container with an appropriate interface package (e.g. argparse). We could do with more explanation as to how to actualy use template, because I don't think we have that here (and the Seqera docs are almost non-existent on the topic).

A PR is in progress to address the gap in the Nextflow docs. This doesn't mean that nf-core can't have its own standards/recommendations, but it will hopefully add something that can be easily referenced.

@SPPearce
Copy link
Contributor

Can we reference the reason why not to use module binaries? Personally I don't really like the interface to template, and I hate to see R scripts written with base R and the horribly clunky code to allow arguments in that way. I would suggest that we encourage the use of a seqera container with an appropriate interface package (e.g. argparse). We could do with more explanation as to how to actualy use template, because I don't think we have that here (and the Seqera docs are almost non-existent on the topic).

A PR is in progress to address the gap in the Nextflow docs. This doesn't mean that nf-core can't have its own standards/recommendations, but it will hopefully add something that can be easily referenced.

Great, always happy to see more documentation on the Nextflow page itself.

Co-authored-by: Christopher Hakkaart <[email protected]>
@jfy133
Copy link
Member

jfy133 commented Dec 10, 2024

Can we reference the reason why not to use module binaries?

Apparently you can only use binaries in cloud contexts if you use wave (not other containers), which I feel is overly restrictive.

Personally I don't really like the interface to template, and I hate to see R scripts written with base R and the horribly clunky code to allow arguments in that way. I would suggest that we encourage the use of a seqera container with an appropriate interface package (e.g. argparse). We could do with more explanation as to how to actualy use template, because I don't think we have that here (and the Seqera docs are almost non-existent on the topic).

Agree, more Nextflow docs/example on how to use would be nice, and again, would rather not 'force' Seqera Containers.

I much prefer the use of in-line scripts where possible, what is the justification behind saying it can only be 20 lines?

It's a rule of thumb suggested by @pinin4fjords (I've changed to 'approx' though)

Oh, and some examples pointing to other modules rather than deseq2/differential repeatedly would be good.

Suggest some then! 😝

@christopher-hakkaart
Copy link
Member

I suggest removing the repeated "approximately 20 lines" and instead describing this once: something like, "When a script extends beyond a readable length (approximately 20 lines)..."

Then just refer to a readable length for the rest of the text.

Readable length could be "Manageable size", "too verbose", "excessive length", or something like that

@jfy133
Copy link
Member

jfy133 commented Dec 10, 2024

I suggest removing the repeated "approximately 20 lines" and instead describing this once: something like, "When a script extends beyond a readable length (approximately 20 lines)..."

Then just refer to a readable length for the rest of the text.

Readable length could be "Manageable size", "too verbose", "excessive length", or something like that

Done!

@christopher-hakkaart
Copy link
Member

Much better - thanks!

@pinin4fjords
Copy link
Member Author

@SPPearce to be clear, I want module binaries to be the way to do this, with argparse baked into the container to avoid all that junk I have to put in templates, I'm no template fanboy.

But module binaries are not entirely portable (see the provisos in the linked docs), so templates are the least worst way of doing this I've found - overjoyed if someone tells me a better way.

My crappy outdated R is another issue entirely ;-)

@christopher-hakkaart
Copy link
Member

Is everyone reasonably happy with the current PR? Are there any blockers to merging?

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.

6 participants