Adding google-nucleus to the REQUIRED_PACKAGES list #416

samanvp · 2018-11-22T20:06:36Z

In PR #346 we added a temporary solution to install Nucleus using its
wheel file. Nucleus now has pip install so we can remove that hack.

Also when running integration tests I was prompted to replace gcloud container builds submit with gcloud builds submit. My first attempt to run the integration tests after that change failed due to build timeout, so I added the --timeout "30" flag.

coveralls · 2018-11-22T20:13:15Z

Pull Request Test Coverage Report for Build 1468

1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
4 unchanged lines in 2 files lost coverage.
Overall coverage increased (+1.4%) to 89.013%

Files with Coverage Reduction	New Missed Lines	%
gcp_variant_transforms/beam_io/vcfio_test.py	2	98.69%
gcp_variant_transforms/beam_io/vcf_parser.py	2	85.95%

Totals
Change from base Build 1467:	1.4%
Covered Lines:	6546
Relevant Lines:	7354

💛 - Coveralls

arostamianfar

Cool! Thanks, Saman! Finally we don't need that hack anymore :)

arostamianfar · 2018-11-22T20:08:57Z

setup.py

@@ -28,6 +26,7 @@
    'google-api-python-client>=1.6',
    'intervaltree>=2.1.0,<2.2.0',
    'pyvcf<0.7.0',
+    'google-nucleus>=0.2.0',


For now, let's actually pin this to 0.2.0 as they may release a new version, which may break our unit tests.

arostamianfar · 2018-11-22T20:12:01Z

gcp_variant_transforms/beam_io/vcfio_test.py

-    vcf_line = ('20	1234	rs123;rs2	C	A,T	50	'
+    # rs123;rs2 -> rs123 it seems nuclues does not parse IDs correctly.
+    # quality=50 -> 50.0 nucleus converts quality values to float.
+    # TODO(samanvp): convert all quality values to float.


@allieychen FYI
I actually noticed that we have the same problem for the BQ -> VCF pipeline. The VCF quality values are typically integers, but the spec has float. So, while technically correct, our output VCF files have #.0 for the quality field.
I think a better solution is to add some logic to our parser to output these as ints if the value is actually an integer (an easy way to do this is: return quality if int(quality) != quality else int(quality) (with some special casing when quality is None). Let's do this in another PR though.

Got it! Thanks!

arostamianfar · 2018-11-22T20:17:15Z

deploy_and_run_tests.sh

@@ -207,8 +207,9 @@ if [[ -z "${skip_build}" ]]; then
  # TODO(bashir2): This will pick and include all directories in the image,
  # including local build and library dirs that do not need to be included.
  # Update this to include only the required files/directories.
-  gcloud container builds submit --config "${build_file}" \
+  gcloud builds submit --config "${build_file}" \


to clarify: is this the new way of using cloud build?

Yes, when I run the deploy_and_run_tests.sh I got the following error message:

ERROR: (gcloud.container.builds.submit) This command has been replaced with `gcloud builds`.

I see. Thanks for fixing this! Could you please update the comment in cloudbuild.yaml as well?

deploy_and_run_tests.sh

samanvp

Integration tests are failing without a meaningful error message:

  File "/tmp/tmp.XPnhTCIT3g/local/lib/python2.7/site-packages/gcp_variant_transforms/testing/integration/run_tests_common.py", line 134, in _handle_failure
    response['error']['message']))
gcp_variant_transforms.testing.integration.run_tests_common.TestCaseFailure: Test case platinum-na12877-hg38-10k-lines (Operation projects/gcp-variant-transforms-test/operations/14235103094015335786) failed: Execution failed: action 1: unexpected exit status 1 was not ignored

It seems this is independent of the platinum-na12877-hg38-10k-lines test case, first another test was failing when I removed its yaml file this test failed with a similar error message.

I am still investigating...

samanvp · 2018-11-22T21:39:33Z

deploy_and_run_tests.sh

@@ -207,8 +207,9 @@ if [[ -z "${skip_build}" ]]; then
  # TODO(bashir2): This will pick and include all directories in the image,
  # including local build and library dirs that do not need to be included.
  # Update this to include only the required files/directories.
-  gcloud container builds submit --config "${build_file}" \
+  gcloud builds submit --config "${build_file}" \


Yes, when I run the deploy_and_run_tests.sh I got the following error message:

ERROR: (gcloud.container.builds.submit) This command has been replaced with `gcloud builds`.

deploy_and_run_tests.sh

samanvp · 2018-11-23T14:13:01Z

setup.py

@@ -28,6 +26,7 @@
    'google-api-python-client>=1.6',
    'intervaltree>=2.1.0,<2.2.0',
    'pyvcf<0.7.0',
+    'google-nucleus>=0.2.0',


arostamianfar

Looks good! Just one nit about the version...
RE integration test failure: it looks like a temporary failure and may go away if you retry. Also, to debug, you need to run gcloud alpha genomics operations describe projects/gcp-variant-transforms-test/operations/14235103094015335786 and look at the logs.

arostamianfar · 2018-11-23T15:16:50Z

deploy_and_run_tests.sh

@@ -207,8 +207,9 @@ if [[ -z "${skip_build}" ]]; then
  # TODO(bashir2): This will pick and include all directories in the image,
  # including local build and library dirs that do not need to be included.
  # Update this to include only the required files/directories.
-  gcloud container builds submit --config "${build_file}" \
+  gcloud builds submit --config "${build_file}" \


I see. Thanks for fixing this! Could you please update the comment in cloudbuild.yaml as well?

arostamianfar · 2018-11-23T15:18:41Z

setup.py

@@ -28,6 +26,7 @@
    'google-api-python-client>=1.6',
    'intervaltree>=2.1.0,<2.2.0',
    'pyvcf<0.7.0',
+    'google-nucleus>=0.2.0,<0.2.1',


Does google-nuclues==0.2.0 not work?

It actually does!
Since there was no other instance of == I assumed it does not work.

samanvp

As suggested by Thomas, adding 'protobuf>=3.6.1' to the list of our requirements solved the problem of failing integration tests. I am not sure whether it's better to pin to version 3.6.1 or we should rely on the latest version?

Please take another look before I submit this PR.

samanvp · 2018-11-23T17:30:22Z

deploy_and_run_tests.sh

@@ -207,8 +207,9 @@ if [[ -z "${skip_build}" ]]; then
  # TODO(bashir2): This will pick and include all directories in the image,
  # including local build and library dirs that do not need to be included.
  # Update this to include only the required files/directories.
-  gcloud container builds submit --config "${build_file}" \
+  gcloud builds submit --config "${build_file}" \


samanvp · 2018-11-23T17:31:30Z

setup.py

@@ -28,6 +26,7 @@
    'google-api-python-client>=1.6',
    'intervaltree>=2.1.0,<2.2.0',
    'pyvcf<0.7.0',
+    'google-nucleus>=0.2.0,<0.2.1',


It actually does!
Since there was no other instance of == I assumed it does not work.

In PR googlegenomics#346 we added a temporary solution to install Nucleus using its wheel file. Nucleus now has pip install so we can remove that hack.

allieychen

Thank you Saman, LGTM!

samanvp requested a review from arostamianfar November 22, 2018 20:06

arostamianfar reviewed Nov 22, 2018

View reviewed changes

samanvp force-pushed the nucleus branch from 016a195 to 5be4039 Compare November 23, 2018 15:03

samanvp commented Nov 23, 2018

View reviewed changes

arostamianfar reviewed Nov 23, 2018

View reviewed changes

samanvp commented Nov 26, 2018

View reviewed changes

samanvp added 2 commits December 4, 2018 09:19

Adding google-nucleus to the REQUIRED_PACKAGES list

c659127

In PR googlegenomics#346 we added a temporary solution to install Nucleus using its wheel file. Nucleus now has pip install so we can remove that hack.

Review comments

a166f85

samanvp force-pushed the nucleus branch from e886484 to a166f85 Compare December 4, 2018 15:44

allieychen reviewed Dec 5, 2018

View reviewed changes

allieychen approved these changes Dec 7, 2018

View reviewed changes

allieychen mentioned this pull request Dec 7, 2018

Minor fix to the test script after gcloud update. #427

Closed

samanvp merged commit 60f36ef into googlegenomics:master Dec 7, 2018

samanvp deleted the nucleus branch September 13, 2019 17:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding google-nucleus to the REQUIRED_PACKAGES list #416

Adding google-nucleus to the REQUIRED_PACKAGES list #416

samanvp commented Nov 22, 2018

coveralls commented Nov 22, 2018 •

edited

Loading

arostamianfar left a comment

arostamianfar Nov 22, 2018

samanvp Nov 23, 2018

arostamianfar Nov 22, 2018

allieychen Nov 23, 2018

arostamianfar Nov 22, 2018

samanvp Nov 22, 2018

arostamianfar Nov 23, 2018

samanvp Nov 23, 2018

samanvp left a comment

samanvp Nov 22, 2018

samanvp Nov 23, 2018

arostamianfar left a comment

arostamianfar Nov 23, 2018

arostamianfar Nov 23, 2018

samanvp Nov 23, 2018

samanvp left a comment

samanvp Nov 23, 2018

samanvp Nov 23, 2018

allieychen left a comment

Adding google-nucleus to the REQUIRED_PACKAGES list #416

Adding google-nucleus to the REQUIRED_PACKAGES list #416

Conversation

samanvp commented Nov 22, 2018

coveralls commented Nov 22, 2018 • edited Loading

Pull Request Test Coverage Report for Build 1468

💛 - Coveralls

arostamianfar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samanvp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arostamianfar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samanvp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allieychen left a comment

Choose a reason for hiding this comment

coveralls commented Nov 22, 2018 •

edited

Loading