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

no more profile-ception #743

Merged
merged 20 commits into from
Sep 15, 2022
Merged

no more profile-ception #743

merged 20 commits into from
Sep 15, 2022

Conversation

maxulysse
Copy link
Member

@drpatelh do you think this is enough?
Just wondering if we can keep our multiple profiles, but just get rid of profile definitions included within profiles.

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/sarek 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).

@github-actions
Copy link

github-actions bot commented Sep 9, 2022 •
edited
Loading

nf-core lint overall result: Passed ✅

Posted for pipeline commit c8f728a

+| ✅ 151 tests passed       |+
#| ❔   4 tests were ignored |#

❔ Tests ignored:

  • files_unchanged - File ignored due to lint config: assets/nf-core-sarek_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-sarek_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-sarek_logo_dark.png
  • files_unchanged - File ignored due to lint config: lib/NfcoreTemplate.groovy

✅ Tests passed:

Run details

  • nf-core/tools version 2.5.1
  • Run at 2022-09-15 10:09:58

FriederikeHanssen
FriederikeHanssen approved these changes Sep 9, 2022
View reviewed changes
Copy link
Contributor

@FriederikeHanssen FriederikeHanssen left a comment

Choose a reason for hiding this comment

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

😭 😭 but has to be done

drpatelh
drpatelh reviewed Sep 9, 2022
View reviewed changes
conf/test_targeted.config Outdated Show resolved Hide resolved
@drpatelh
Copy link
Member

drpatelh commented Sep 9, 2022

I can confirm that testing this branch is working as expected on Tower.

  • Each of the individual profiles are now available for users to select via the Pipeline settings:

    image

  • The parameters are resolved as expected in the Parameters page:

    image

Thank you!

@maxulysse
Copy link
Member Author

So regarding the issue with the controlfreec CI test.
Not sure what is happening there, or actually I finally figured what is happening, but I'm not sure I have an explanation on the how, why or even why this wasn't happening before.

Basically the process scope definition in the conf/test.config is overwritten by the one in the conf/modules.config which wasn't happening before...
And I have no idea why...
I'm so confused by this error to be honest.

I did find a solution which was to delay the loading of the test config files over the module config file.
But not super happy with that solution, as I figured we had our reasoning for the order of the inclusion of said files...

What do you think @drpatelh @FriederikeHanssen

@maxulysse
Copy link
Member Author

I did find a solution which was to delay the loading of the test config files over the module config file. But not super happy with that solution, as I figured we had our reasoning for the order of the inclusion of said files...

So this did not work...
(It did fix the issue I had, but created more...)
So I'm moving around just the relevant part for now as a patch, and we'll figure things out better another time

FriederikeHanssen
FriederikeHanssen approved these changes Sep 14, 2022
View reviewed changes
Copy link
Contributor

@FriederikeHanssen FriederikeHanssen left a comment

Choose a reason for hiding this comment

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

🚀 still don't understand the error

@maxulysse
Copy link
Member Author

Last commit feels the less patchy way to make it work.
@drpatelh does loading 2 files for the same profile in 2 different scope in the same file works well with tower?

@FriederikeHanssen we'll need to fix the somatic ascat profile, but I'm now more happy with this PR

drpatelh
drpatelh reviewed Sep 14, 2022
View reviewed changes
nextflow.config Outdated Show resolved Hide resolved
@maxulysse maxulysse merged commit 43d4a06 into nf-core:dev Sep 15, 2022
@maxulysse maxulysse deleted the dev_test_profiles branch September 15, 2022 11:05
@maxulysse
Copy link
Member Author

maxulysse commented Oct 11, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drpatelh drpatelh drpatelh left review comments

@FriederikeHanssen FriederikeHanssen Awaiting requested review from FriederikeHanssen FriederikeHanssen is a code owner

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

Successfully merging this pull request may close these issues.
3 participants
@maxulysse @drpatelh @FriederikeHanssen