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-1574] Add merge and decode to staging #161

Merged
merged 19 commits into from
Nov 1, 2024

Conversation

rxu17
Copy link
Contributor

@rxu17 rxu17 commented Oct 28, 2024

Purpose: We currently don't have a way to safely test and upload results to non-production environment for merge_and_decode step in genie bpc pipeline. This is to add that.

Changes:
Aside from the nextflow workflow changes, here are the relevant changes in the merge_and_uncode_rca_uploads.R script:

  • New helper function get_output_folder_id to be used to retrieve the output synapse id
  • New production flag and logic surrounding it to retrieve the correct entities for staging vs production
  • New comment flag NOTE: this is just a placeholder, we will need to update R and synapser drastically to be able to use this, and that's beyond the scope of this ticket.

Testing:

  • Added unit testing for helper function get_output_folder_id
  • Tested on staging project (locally and on tower). Comparison results for a couple of cohorts saved to JIRA ticket
  • Not a perfect integration test but ran on develop branch docker (except changed the synapse id of the output to staging) and compared it to results on the feature branch docker and no differences found!

@rxu17 rxu17 changed the title Gen 1574 merge and decode staging [GEN-1574] Add merge and decode to staging Oct 28, 2024
@rxu17 rxu17 marked this pull request as ready for review October 29, 2024 21:01
@rxu17 rxu17 requested a review from a team as a code owner October 29, 2024 21:01
Copy link
Contributor

@danlu1 danlu1 left a comment

Choose a reason for hiding this comment

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

Great work. Leave a few minor comments.

scripts/uploads/merge_and_uncode_rca_uploads.R Outdated Show resolved Hide resolved
scripts/uploads/merge_and_uncode_rca_uploads.R Outdated Show resolved Hide resolved
scripts/uploads/merge_and_uncode_rca_uploads.R Outdated Show resolved Hide resolved
Copy link
Contributor

@danlu1 danlu1 left a comment

Choose a reason for hiding this comment

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

LTGM!

@@ -13,7 +13,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
module: ["references", "table_updates"] # Define the modules you want to loop through for builds
module: ["references", "table_updates", "uploads"] # Define the modules you want to loop through for builds
Copy link
Member

Choose a reason for hiding this comment

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

This is wonderful! Yay for scalable solutions!

Comment on lines +700 to +701
make_option(c("-a", "--synapse_auth"), type = "character", default = NA,
help="Path to .synapseConfig file or Synapse PAT (default: normal synapse login behavior)"),
Copy link
Member

Choose a reason for hiding this comment

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

Nit: don't need to do this now, but in the future if you see these, it can all be removed since we added support SYNAPSE_AUTH_TOKEN style login to the synapse client.

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!

@rxu17 rxu17 merged commit 24a49b7 into develop Nov 1, 2024
4 checks passed
@rxu17 rxu17 deleted the gen-1574-merge-and-decode-staging branch November 1, 2024 04:35
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.

3 participants