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

fix: Balsamic container & pytests (fix cyvcf2 version) #1206

Merged
merged 7 commits into from Jul 21, 2023

Conversation

ivadym
Copy link
Contributor

@ivadym ivadym commented Jul 19, 2023

This PR:

The recent Cython update to v0.30 has led to compilation errors, resulting in the failure to execute the Balsamic container and, more importantly, preventing pytest from running via GitHub actions.

Screenshot 2023-07-19 at 14 51 37

Changed:

  • Fix cyvcf2 to version 0.30.22

Review and tests:

  • Tests pass
    Screenshot 2023-07-20 at 13 40 04
  • Code review
  • New code is executed and covered by tests, and test approve

@ivadym ivadym requested a review from a team July 19, 2023 12:55
@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

❗ No coverage uploaded for pull request base (release_12.0.2@6de84eb). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@                Coverage Diff                @@
##             release_12.0.2    #1206   +/-   ##
=================================================
  Coverage                  ?   99.31%           
=================================================
  Files                     ?       29           
  Lines                     ?     1756           
  Branches                  ?        0           
=================================================
  Hits                      ?     1744           
  Misses                    ?       12           
  Partials                  ?        0           
Flag Coverage Δ
unittests 99.31% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@mathiasbio mathiasbio left a comment

Choose a reason for hiding this comment

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

Sorry for the slow review! I have no comments about the code.

For the cyvcf2 changes, I'm not sure if I understand the changes correctly. If there has been no change to the version you can ignore my comments below.

If the version has been changed I'm wondering if any tests are required to make sure that the following scripts are working properly, and do not affect the results in any negative unforeseen way.

from cyvcf2 import VCF

from cyvcf2 import VCF, Writer

It seems for instance that edit_vcf_info.py is used in varcaller_filter_tumor_only.rule, and in varcaller_filter_tumor_normal.rule

And it could be nice to confirm that the VCF-files are not affected in any major way, just to avoid the possibility of needing to validate for TGA too in this small patch, since these rules affect the TGA workflow.

For the vcfutil plugin I don't know where it's used exactly! But similarly there, have we considered effects on this plugin based on the changes?

@mathiasbio mathiasbio self-requested a review July 21, 2023 10:09
Copy link
Collaborator

@mathiasbio mathiasbio left a comment

Choose a reason for hiding this comment

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

Meant to approve along with the comments.
Nice fix as always! The hero we don't deserve but are happy we have regardless ⭐ 🧙 ⭐

@ivadym
Copy link
Contributor Author

ivadym commented Jul 21, 2023

Sorry for the slow review! I have no comments about the code.

For the cyvcf2 changes, I'm not sure if I understand the changes correctly. If there has been no change to the version you can ignore my comments below.

If the version has been changed I'm wondering if any tests are required to make sure that the following scripts are working properly, and do not affect the results in any negative unforeseen way.

from cyvcf2 import VCF

from cyvcf2 import VCF, Writer

It seems for instance that edit_vcf_info.py is used in varcaller_filter_tumor_only.rule, and in varcaller_filter_tumor_normal.rule

And it could be nice to confirm that the VCF-files are not affected in any major way, just to avoid the possibility of needing to validate for TGA too in this small patch, since these rules affect the TGA workflow.

For the vcfutil plugin I don't know where it's used exactly! But similarly there, have we considered effects on this plugin based on the changes?

Thanks for the review 💟

We need to validate the entire workflow as we had to address issues with the balsamic container by fixing the pydantic version. I think it's fine with the integration tests.

Regarding vcfutil.py, we are only using it for createvcf() method, which is actually not being used anywhere except for the pytests.

But we're using it for edit_vcf_info.py as you said, but I'm hoping that the pytest we have are reliable enough. Doesn't seem we have breaking changes with the new version of cyvcf2, and I would say we validate it as part of the integration tests

@ivadym ivadym changed the base branch from master to release_12.0.2 July 21, 2023 10:41
@sonarcloud
Copy link

sonarcloud bot commented Jul 21, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@ivadym ivadym merged commit 2149b56 into release_12.0.2 Jul 21, 2023
19 of 20 checks passed
@ivadym ivadym deleted the fix-cyvcf2-version branch July 21, 2023 10:52
This was referenced Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants