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.

Sign up for GitHub

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

Jump to bottom

Fix fasta input #544

Merged
merged 7 commits into from Feb 27, 2023
Merged

Fix fasta input #544

merged 7 commits into from Feb 27, 2023

Conversation

jtangrot
Copy link
Contributor

This PR addresses #542. A module was added to change tabs to spaces in description part of sequence names when using fasta input. The descriptions are kept as part of the sequence name. Removing the tabs will remove the problem with the sequence on a line of its own.

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the pipeline conventions in the contribution docs- [ ] If necessary, also make a PR on the nf-core/ampliseq branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@jtangrot
Copy link
Contributor Author

I have added descriptions to some of the file names in the fasta file used for testing. I don't know what else needs changing in the testing?

@github-actions
Copy link

github-actions bot commented Feb 24, 2023 •
edited

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 83aca46

+| ✅ 155 tests passed       |+
#| ❔   1 tests were ignored |#
!| ❗   2 tests had warnings |!

❗ Test warnings:

  • readme - README did not have a Nextflow minimum version badge.
  • schema_lint - Parameter input is not defined in the correct subschema (input_output_options)

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.7.2
  • Run at 2023-02-27 13:19:50

@erikrikarddaniel
Copy link
Member

I have added descriptions to some of the file names in the fasta file used for testing. I don't know what else needs changing in the testing?

Perfect.

@erikrikarddaniel
Copy link
Member

There are tests failing, so I'm waiting with my review. Need to go on an errand now.

d4straub
d4straub reviewed Feb 24, 2023
View reviewed changes
Copy link
Collaborator

@d4straub d4straub left a comment

Choose a reason for hiding this comment

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

Looks good to me, I'll restart tests, maybe the errors are on github (since they seem unrelated to your changes).

CHANGELOG.md Outdated Show resolved Hide resolved
@d4straub
Copy link
Collaborator

There is a reproducible (here in github) error with -profile test,docker. The error message is

No such variable: info

 -- Check script './workflows/../subworkflows/local/parse_input.nf' at line: 63 or see '.nextflow.log' file for more details
Argument of 'channel.from' method cannot be a channel object — Likely you can replace the use of 'channel.from' with the channel object itself

 -- Check script './workflows/ampliseq.nf' at line: 652 or see '.nextflow.log' file for more details

I do not understand whats happening though.

@erikrikarddaniel
Copy link
Member

There is a reproducible (here in github) error with -profile test,docker. The error message is

No such variable: info

 -- Check script './workflows/../subworkflows/local/parse_input.nf' at line: 63 or see '.nextflow.log' file for more details
Argument of 'channel.from' method cannot be a channel object — Likely you can replace the use of 'channel.from' with the channel object itself

 -- Check script './workflows/ampliseq.nf' at line: 652 or see '.nextflow.log' file for more details

I do not understand whats happening though.

The error message is clear enough -- and Channel.from(ch_multiqc_config) looks like it could be the offender, since it's calling the method with a channel (possibly empty) -- but strange that it only affects docker and shows up in a file that's not touched. It's input can have been touched of course, but still strange that singularity works -- or does it (I don't really understand this test)?

@d4straub
Copy link
Collaborator

Well, that works for me flawless:

nextflow pull jtangrot/ampliseq
nextflow pull jtangrot/ampliseq -r fix_fasta_input
nextflow run jtangrot/ampliseq -r fix_fasta_input -profile test,singularity --outdir results_jtangrot-fix_fasta_input__test
nextflow run jtangrot/ampliseq -r fix_fasta_input -profile test,docker --outdir results_jtangrot-fix_fasta_input__test_docker

@jtangrot
Copy link
Contributor Author

Thanks for looking into this. I really have no clue how to fix this...

@d4straub
Copy link
Collaborator

Maybe bother the github actions experts in the nf-core slack?

@d4straub
Copy link
Collaborator

The failing test nf-core CI / Run pipeline with test data (latest-everything) (pull_request) uses N E X T F L O W ~ version 23.02.0-edge, which was released 3 days ago. I strongly suspect that the nextflow version causes that issue. If that is true, then we can ignore the failing test for that PR. I can check that on Monday.

@erikrikarddaniel
Copy link
Member

Or maybe fix the problem so it works with the new version? Sooner or later we might have to anyway.

@erikrikarddaniel
Copy link
Member

Did the test get stuck? I think this looks fine with the change to the channel, but wanted to wait until the test completed.

@d4straub
Copy link
Collaborator

NXF_VER=23.02.0-edge nextflow run nf-core/ampliseq -r 2.4.1 -profile test,singularity leads to the same error, therefore confirmed that its the nextflow version, not this PR.

Or maybe fix the problem so it works with the new version? Sooner or later we might have to anyway.

Thats an idea, but because it seems to have nothing to do with this PR, I was rather leaning towards fixing this separately. Anyway, the latest commit up there is only addressing the problem partially I think. It seems we deviate from the nf-core template 2.7.2, i.e. the whole line might have to be removed?
I guess that was my mistake when doing the template update.

@erikrikarddaniel
Copy link
Member

I'm fine with either approving this latest commit or making a fuller attempt at addressing issues from an upgrade of Nextflow.

d4straub
d4straub reviewed Feb 27, 2023
View reviewed changes
workflows/ampliseq.nf Outdated Show resolved Hide resolved
d4straub and others added 2 commits February 27, 2023 13:45
@d4straub
fix ch_multiqc_config
4e67ad6
@jtangrot @d4straub
Update CHANGELOG.md
83aca46 
Co-authored-by: Daniel Straub <42973691+d4straub@users.noreply.github.com>
d4straub
d4straub approved these changes Feb 27, 2023
View reviewed changes
Copy link
Collaborator

@d4straub d4straub left a comment

Choose a reason for hiding this comment

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

LGTM!

@jtangrot
Copy link
Contributor Author

Thanks!

@jtangrot jtangrot merged commit b41dac6 into nf-core:dev Feb 27, 2023
@jtangrot jtangrot deleted the fix_fasta_input branch February 27, 2023 13:56
@jtangrot jtangrot mentioned this pull request Feb 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erikrikarddaniel erikrikarddaniel erikrikarddaniel left review comments

@d4straub d4straub d4straub approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jtangrot @erikrikarddaniel @d4straub