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

365 vat python testing additions #7756

Merged
merged 18 commits into from
May 3, 2022
Merged

Conversation

RoriCremer
Copy link
Contributor

@RoriCremer RoriCremer commented Apr 5, 2022

Docker image added---I think we'll want to implement whatever we are doing for the jars for the docker images in parallel

I'm planning to add an additional python test to this and then I want to dig into how to make sure they run during docker build

outside the scope for this specific round of python testing, but it will be worth reaching out the the coworker who spotted both previous clinvar issues to ask for additional spot testing cases

@RoriCremer RoriCremer force-pushed the rc-vat-clinvar-updates branch 3 times, most recently from 2725442 to 315c4c8 Compare April 7, 2022 17:37
@RoriCremer RoriCremer force-pushed the rc-vat-clinvar-updates branch 6 times, most recently from 2653fc1 to 1ff2d91 Compare April 28, 2022 00:17
@codecov
Copy link

codecov bot commented Apr 28, 2022

Codecov Report

❗ No coverage uploaded for pull request base (ah_var_store@75b5115). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             ah_var_store     #7756   +/-   ##
================================================
  Coverage                ?   86.305%           
  Complexity              ?     35190           
================================================
  Files                   ?      2170           
  Lines                   ?    164837           
  Branches                ?     17775           
================================================
  Hits                    ?    142262           
  Misses                  ?     16253           
  Partials                ?      6322           

@RoriCremer RoriCremer changed the title 365 vat clinvar updates 365 vat python testing additions Apr 28, 2022
Copy link

@rsasch rsasch left a comment

Choose a reason for hiding this comment

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

I tried running ./build_docker.sh and it failed with the following error:

ImportError: Failed to import test module: create_variant_annotation_table_test
Traceback (most recent call last):
  File "/opt/miniconda3/envs/gvs/lib/python3.8/unittest/loader.py", line 154, in loadTestsFromName
    module = __import__(module_name)
  File "/Users/rasch/p/gatk/scripts/variantstore/wdl/extract/create_variant_annotation_table_test.py", line 13, in <module>
    from create_variant_annotation_table import make_annotated_json_row
  File "/Users/rasch/p/gatk/scripts/variantstore/wdl/extract/create_variant_annotation_table.py", line 8, in <module>
    import ijson
ModuleNotFoundError: No module named 'ijson'

@RoriCremer
Copy link
Contributor Author

oooh, re: ijson, should I just add checking for and installing it into the ./build_docker.sh file?

I've had it forever since I need it to run the python script. It's in requirements.txt, where else should I put it?

@rsasch
Copy link

rsasch commented Apr 29, 2022

I think it would be good for there to be as little lift as possible required to run the tests, so adding it to ./build_docker.sh is probably best.

scripts/variantstore/wdl/GvsCreateVATFromAnnotations.wdl Outdated Show resolved Hide resolved
scripts/variantstore/wdl/extract/build_docker.sh Outdated Show resolved Hide resolved
row_alt="A",
variant_line=cysticFibrosisNirvanaOutput,
transcript_line=None)
## This is dumb....I do this because json and python dicts aren't playing nice
Copy link
Collaborator

Choose a reason for hiding this comment

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

dumb question probably but I don't follow why this is needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Python dicts use None and json uses null and the testing doesn't seem to be able to reconcile.

Copy link
Collaborator

@mcovarr mcovarr left a comment

Choose a reason for hiding this comment

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

a couple of Python nitpicks, otherwise looks good

row_alt="A",
variant_line=cysticFibrosisNirvanaOutput,
transcript_line=None)
## This is dumb....I do this because json and python dicts aren't playing nice
Copy link
Collaborator

Choose a reason for hiding this comment

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

This shouldn't be necessary, the Python json library should handle this automatically. The issue appears to be that the "expected" files have the values that are meant to be nulls wrapped in quotes so json (correctly) treats them as strings. e.g.

  "gnomad_nfr_ac": "null",

should be

  "gnomad_nfr_ac": null,

@RoriCremer
Copy link
Contributor Author

Bec's change resolved!

@RoriCremer RoriCremer merged commit d77ebf5 into ah_var_store May 3, 2022
@RoriCremer RoriCremer deleted the rc-vat-clinvar-updates branch May 3, 2022 16:41
This was referenced Mar 17, 2023
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