-
Notifications
You must be signed in to change notification settings - Fork 1
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-863] Add patch release #21
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good logically!
…ion workload to reduce parameters
Co-authored-by: BryanFauble <[email protected]>
…orkflows/nf-genie into modularize-patch-code
…ch-code [GEN-863] Modularize patch code by splitting off patch_file function
🎉 All dependencies have been resolved ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I have a few comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we want to include a new module to call this compare_patch
script?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rxu17 I just realized, this might not be super useful, because the compare_patch
script is more of an integration test for testing given data in, ensure data out.
What we probably want as the comparison module is what you wrote for main GENIE and/or BPC.
That said, I still added the comparison code to run when it's a staging run, the only issue is that it only works with the default parameters of the nextflow workflow
Although that makes me wonder if I should add this at all - thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What did you mean by this? In regards to staging pipeline run
I still added the comparison code to run when it's a staging run, the only issue is that it only works with the default parameters of the nextflow workflow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compare patch code currently compares data against an existing public patch release. So for example, I recreated the 15.X public patch release here, and I compared it against the existing public patch release to see if I made any incorrect code changes.
However, when I create a new patch release, there's not exactly an existing patch release to compare it to. Therefore, this would act more like an integration tests against an old public release (assuming the logic doesn't change)
What we probably want in the future, is a comparison of sample count in vs sample count out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compare patch code currently compares data against an existing public patch release. So for example, I recreated the 15.X public patch release here, and I compared it against the existing public patch release to see if I made any incorrect code changes.
However, when I create a new patch release, there's not exactly an existing patch release to compare it to. Therefore, this would act more like an integration tests against an old public release (assuming the logic doesn't change)
What we probably want in the future, is a comparison of sample count in vs sample count out.
Quality Gate passedIssues Measures |
Create patch release nextflow workflow. Depends on #22
The two folders compared are 15.5-consortium (syn55146141) and 15.6-consortium (staging: syn62069187). Here is the output:
I then manually downloaded the 15.5-consortium and 15.6-consortium sample and patient files, removed the comment headers and checked the md5.
Before removing comments:
After removing comments:
That is proof that the file is changed due to the NAACCR mappings updated