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

Use relative paths for inputs to make the training portable #438

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

Conversation

adamrtalbot
Copy link
Collaborator

The training encoded an absolute path in the samplesheet. This broke the
training when ran on a devcontainer. Importantly, Gitpod will be
deprecating their custom format in April 2025 and using devcontainers
so even if we don't merge this PR it will help indicate what we need to
change.

The training encoded an absolute path in the samplesheet. This broke the
training when ran on a devcontainer. Importantly, Gitpod will be
deprecating their custom format in April 2025 and using devcontainers
so even if we don't merge this PR it will help indicate what we need to
change.
Copy link

netlify bot commented Nov 6, 2024

Deploy Preview for nextflow-training ready!

Name Link
🔨 Latest commit 389e560
🔍 Latest deploy log https://app.netlify.com/sites/nextflow-training/deploys/673a2bb5dd23390008468162
😎 Deploy Preview https://deploy-preview-438--nextflow-training.netlify.app
📱 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.

Comment on lines 112 to 114
reads_ch = Channel.fromPath(params.reads_bam)
.splitText()
.map { it.trim() }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Annoyingly, we need a trim() in here to make it work. I'm not sure why we need this, but this is the simplest fix I can find.

Copy link
Member

Choose a reason for hiding this comment

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

@adamrtalbot Can this be replaced by a .splitCsv() . From what I recall it doesn't need the trim.

Also, if we need to keep the it.trim() we are moving away from using the implicit it variable and should instead used named variables in our closures. It's much less confusing to newbies and will become mandatory in a future nextflow version according to Ben.

Comment on lines 773 to 774
reads_ch = Channel.fromPath(params.reads_bam)
.splitText() { bamPath -> file(bamPath.trim()) }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does anyone know if there's a better way of doing this? Without the map nothing works 😠

Copy link
Member

Choose a reason for hiding this comment

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

splitCsv?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the equivalent splitCsv():

reads_ch = Channel.fromPath(params.reads_bam)
                .splitCsv()
                .map { bamPath -> file(bamPath[0]) }

I think it looks nicer

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. I like that version a lot. It's a lot easier to follow.

@mribeirodantas
Copy link
Member

One thing we should keep in mind is that, at least for now, it's the same cloud environment for all training materials. We set a default start working directory based on our preferences at the time, but we should use absolute paths in some places to make sure people will be at the right place according to what training they're doing.

Use splitCsv to parse the sample_bams samplesheet, which means we do not need to strip the newline characters from the file paths and can use relative paths.
@adamrtalbot adamrtalbot force-pushed the use_relative_paths_for_inputs branch from f46674c to 603e7ac Compare November 7, 2024 09:53
Comment on lines +720 to +722
/data/bam/reads_mother.bam
/data/bam/reads_father.bam
/data/bam/reads_son.bam
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/data/bam/reads_mother.bam
/data/bam/reads_father.bam
/data/bam/reads_son.bam
./data/bam/reads_mother.bam
./data/bam/reads_father.bam
./data/bam/reads_son.bam

```

This should show `/workspaces/training/hello-nextflow`. The important point is the `hello-nextflow` is the final path.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
This should show `/workspaces/training/hello-nextflow`. The important point is the `hello-nextflow` is the final path.
This should show `/workspaces/hello-nextflow`. The important point is the `hello-nextflow` is the final path.

@pinin4fjords
Copy link
Collaborator

See #462 for simpler short-term workaround

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.

4 participants