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

[GEN-1163] hotfix annotation tools #9

Merged
merged 2 commits into from
Feb 17, 2024
Merged

Conversation

rxu17
Copy link

@rxu17 rxu17 commented Feb 16, 2024

Purpose: This PR fix uses a universal way to get the current directory of the script. Mainly needed because our nextflow script calls the python script (inside Genie repo) that calls the bash script inside the annotation-tools repo

Testing:

  • Tested locally on genie pipeline
  • Tested on nf-genie using rebuilt docker image that contains this fix, and process_maf runs all the way through

@rxu17 rxu17 marked this pull request as ready for review February 16, 2024 23:21
@rxu17 rxu17 requested a review from a team as a code owner February 16, 2024 23:21
Copy link
Member

@thomasyu888 thomasyu888 left a comment

Choose a reason for hiding this comment

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

🔥 LGTM! You will have to rebuild the GENIE repo. A push to master branch with a change in the readme would probably do it.

@rxu17
Copy link
Author

rxu17 commented Feb 17, 2024

🔥 LGTM! You will have to rebuild the GENIE repo. A push to master branch with a change in the readme would probably do it.

Since I have to release a new version of annotation-tools and update the main genie Dockerfile, did you want me to update the Dockerfile and push that to master directly?

@thomasyu888
Copy link
Member

thomasyu888 commented Feb 17, 2024

🔥 LGTM! You will have to rebuild the GENIE repo. A push to master branch with a change in the readme would probably do it.

Since I have to release a new version of annotation-tools and update the main genie Dockerfile, did you want me to update the Dockerfile and push that to master directly?

Ah I forgot about the fact that we tagged releases now. Yes feel free to open a PR directly to master and I'll review.

@rxu17
Copy link
Author

rxu17 commented Feb 17, 2024

Ah I forgot about the fact that we tagged releases now. Yes feel free to open a PR directly to master and I'll review.

To be safe, I'm going to wait until I am a part of the genieadmin DockerHub team (asking IT on Monday) so I could build and push the test docker and have the Nextflow Tower workflow pull that to run it on the test pipeline. I'm not 100% positive that running nextflow locally vs via Tower will resolve file paths the same way even though theoretically they should be the same. Also hopefully then just getting the test pipeline on Tower to run all the way through (no issues) will mean similar smoothness for production. Hopefully that delay is ok.

@thomasyu888
Copy link
Member

thomasyu888 commented Feb 17, 2024

Ah I forgot about the fact that we tagged releases now. Yes feel free to open a PR directly to master and I'll review.

To be safe, I'm going to wait until I am a part of the genieadmin DockerHub team (asking IT on Monday) so I could build and push the test docker and have the Nextflow Tower workflow pull that to run it on the test pipeline. I'm not 100% positive that running nextflow locally vs via Tower will resolve file paths the same way even though theoretically they should be the same. Also hopefully then just getting the test pipeline on Tower to run all the way through (no issues) will mean similar smoothness for production. Hopefully that delay is ok.

The delay is ok! But we're going to run into problems with dockerhub because we don't have any more seats. The fastest way would be to build and push to your own dockerhub repo. We need to push to GHCR so we avoid the whole dockerhub situation...

Edit: alternatively, pushing to the dev branch will trigger a different tag to be created. So you can use that.

@rxu17 rxu17 merged commit bc9e00f into master Feb 17, 2024
1 check passed
@rxu17 rxu17 deleted the gen-1163-hotfix-annotation-tools branch February 17, 2024 10:18
@rxu17
Copy link
Author

rxu17 commented Feb 17, 2024

Tested on Nextflow Tower : Ran on test-bash branch of nf-genie which uses the docker container on my personal github (rxu153/test-nf-genie:latest) that checks out the gen-1163-hotfix-annotation-tools branch of annotation-tools to use. See successful run on process_maf here: https://tower.sagebionetworks.org/orgs/Sage-Bionetworks/workspaces/genie-bpc-project/watch/1HTXq5rHaAvab8

Side note: Not sure why the create_data_guide module is failing on the test pipeline? Is this expected

@thomasyu888
Copy link
Member

Tested on Nextflow Tower : Ran on test-bash branch of nf-genie which uses the docker container on my personal github (rxu153/test-nf-genie:latest) that checks out the gen-1163-hotfix-annotation-tools branch of annotation-tools to use. See successful run on process_maf here: https://tower.sagebionetworks.org/orgs/Sage-Bionetworks/workspaces/genie-bpc-project/watch/1HTXq5rHaAvab8

Side note: Not sure why the create_data_guide module is failing on the test pipeline? Is this expected

Thanks for doing that - LGTM!

That is expected because it's missing a file here. We can add it to get a thorough run through https://github.com/Sage-Bionetworks-Workflows/nf-genie/tree/main/scripts%2Fdata_guide%2Fgenomic_profiles

PS. I appreciate the urgency, but please enjoy your weekend! We can pick this up next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants