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

Removing docker.userEmulation #1405

Merged
merged 12 commits into from
Feb 23, 2024
Merged

Removing docker.userEmulation #1405

merged 12 commits into from
Feb 23, 2024

Conversation

asp8200
Copy link
Contributor

@asp8200 asp8200 commented Feb 14, 2024 •
edited
Loading

#1404.

I wonder if this thing with docker.userEmulation and GATK-spark is tested by the CI-tests 🤔

TO-DO: Update changelog. Remove silly tag foo.

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 14, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit ac80ab9

+| ✅ 182 tests passed       |+
#| ❔  13 tests were ignored |#
!| ❗   4 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in WorkflowSarek.groovy: Optionally add in-text citation tools to this list.
  • pipeline_todos - TODO string in main.nf: Optionally add in-text citation tools to this list.
  • pipeline_todos - TODO string in main.nf: Optionally add bibliographic entries to this list.
  • pipeline_todos - TODO string in main.nf: Only uncomment below if logic in toolCitationText/toolBibliographyText has been filled!

❔ Tests ignored:

  • files_exist - File is ignored: .github/workflows/awsfulltest.yml
  • files_exist - File is ignored: .github/workflows/awstest.yml
  • files_exist - File is ignored: conf/modules.config
  • files_exist - File is ignored: lib/WorkflowMain.groovy
  • files_exist - File is ignored: lib/NfcoreTemplate.groovy
  • files_exist - File is ignored: lib/WorkflowSarek.groovy
  • 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: .gitignore or .prettierignore or pyproject.toml
  • actions_ci - actions_ci
  • actions_awstest - 'awstest.yml' workflow not found: /home/runner/work/sarek/sarek/.github/workflows/awstest.yml
  • template_strings - template_strings

✅ Tests passed:

Run details

  • nf-core/tools version 2.13
  • Run at 2024-02-23 12:45:09

@maxulysse
Copy link
Member

Looks good to me, we might need to figure out what to do for spark

@asp8200
Copy link
Contributor Author

asp8200 commented Feb 14, 2024

Looks good to me, we might need to figure out what to do for spark

Ok, but we take care of spark in another PR?

@maxulysse
Copy link
Member

Looks good to me, we might need to figure out what to do for spark

Ok, but we take care of spark in another PR?

let's make an issue for it then

@FriederikeHanssen
Copy link
Contributor

ahhhhhhhhhhh so much time spent on this. This was the only way we could ever get it to work reliably. there are loads of slack threads on it. I think there was even a discussion on the nextflow repo that this will hurt us :D

@asp8200 asp8200 mentioned this pull request Feb 14, 2024
Closed
@maxulysse
Copy link
Member

ahhhhhhhhhhh so much time spent on this. This was the only way we could ever get it to work reliably. there are loads of slack threads on it. I think there was even a discussion on the nextflow repo that this will hurt us :D

yeah, but it's deprecated, so not sure what to do...

@asp8200
Copy link
Contributor Author

asp8200 commented Feb 14, 2024

I removed it because I was getting this: WARN: Undocumented setting "docker.userEmulation" is not supported any more - please remove it from your config. I also removed it in dev-bamtofastq.

@FriederikeHanssen
Copy link
Contributor

Can we ask in the discourse/seqera/nextflow help channel? If this breaks stuff it would be great if there is something superseding it

@asp8200
Copy link
Contributor Author

asp8200 commented Feb 14, 2024 •
edited
Loading

Can we ask in the discourse/seqera/nextflow help channel? If this breaks stuff it would be great if there is something superseding it

Sure. How about you ask, Rike? You seem to know a thing or two about this.

Is Spark only a thing for GATK?

I just tried running this test:

nextflow run main.nf -profile test_cache,use_gatk_spark,docker --outdir foo

on my branch asp8200:remove_docker_userEmulation and the pipeline didn't crash :-) What was the problem with Nextflow, Sarek and Spark?

@FriederikeHanssen
Copy link
Contributor

ok so there actually isn't an issue? spark is tested on CI

@FriederikeHanssen
Copy link
Contributor

this is one of those changes though where I would like to see all tests to run

@asp8200
Copy link
Contributor Author

asp8200 commented Feb 14, 2024

this is one of those changes though where I would like to see all tests to run

Yes, it would be nice to see all tests pass. How do we trigger them?

@maxulysse
Copy link
Member

this is one of those changes though where I would like to see all tests to run

Yes, it would be nice to see all tests pass. How do we trigger them?

make a dummy change in a gatk spark file, and the tests should be triggered

@matthdsm
Copy link

Instead of setting userEmulation to true, you should set the runOptions to

docker.runOptions = '--platform=linux/amd64 -e "HOME=${HOME}" -v /etc/passwd:/etc/passwd:ro -v /etc/shadow:/etc/shadow:ro -v /etc/group:/etc/group:ro -v $HOME:$HOME'

to get the same behavior.

@asp8200
Copy link
Contributor Author

asp8200 commented Feb 22, 2024 •
edited
Loading

Instead of setting userEmulation to true, you should set the runOptions to

docker.runOptions = '--platform=linux/amd64 -e "HOME=${HOME}" -v /etc/passwd:/etc/passwd:ro -v /etc/shadow:/etc/shadow:ro -v /etc/group:/etc/group:ro -v $HOME:$HOME'

to get the same behavior.

Thanks for the suggestion, @matthdsm. However, I wonder if it is necessary to add those runOptions for docker. I just ran

nextflow run main.nf -profile test_cache,use_gatk_spark,docker --outdir results

with nextflow 24.01.0-edge and docker 20.10.21.

@asp8200
Copy link
Contributor Author

asp8200 commented Feb 22, 2024

The joint_germline test is also failing with Matthias' addition to docker.runOptions. Note that joint_germline is not only failing for "NF latest" but also for "NF 23.04.0".

maxulysse
maxulysse reviewed Feb 23, 2024
View reviewed changes
nextflow.config Outdated Show resolved Hide resolved
FriederikeHanssen
FriederikeHanssen reviewed Feb 23, 2024
View reviewed changes
nextflow.config Outdated Show resolved Hide resolved
@maxulysse @FriederikeHanssen
Apply suggestions from code review
5238ddc 
Co-authored-by: Friederike Hanssen <Friederike.hanssen@qbic.uni-tuebingen.de>
@maxulysse
Copy link
Member

Let's update CHANGELOG and merge

@maxulysse maxulysse marked this pull request as ready for review February 23, 2024 12:21
@maxulysse maxulysse merged commit 12d0867 into nf-core:dev Feb 23, 2024
21 checks passed
@asp8200
Copy link
Contributor Author

asp8200 commented Feb 27, 2024

Apologies, guys 🙏 I didn't test @matthdsm's suggest with the docker.runOptions properly. (As far as I recall, I just test it in nextflow.config, but I guess I should have also set it in test.config and cache.config.)

I got some help from @mahesh-panchal on this, and he recommended setting docker.runOptions to

-u $(id -u):$(id -g) -v /etc/passwd:/etc/passwd:ro -v /etc/shadow:/etc/shadow:ro -v /etc/group:/etc/group:ro

I'm updating the docker.runOptions in this PR.

@asp8200 asp8200 mentioned this pull request Feb 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FriederikeHanssen FriederikeHanssen FriederikeHanssen left review comments

@matthdsm matthdsm matthdsm approved these changes

@maxulysse maxulysse maxulysse 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.
4 participants
@asp8200 @maxulysse @FriederikeHanssen @matthdsm