retry on failure #381

allieychen · 2018-10-16T14:41:12Z

When there are a large number of blobs, the composing may fail. Retry when this happens.

Issues: 85
Tested: manually ran BQ to VCF.

coveralls · 2018-10-17T14:32:04Z

Pull Request Test Coverage Report for Build 1384

3 of 26 (11.54%) changed or added relevant lines in 1 file are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.2%) to 87.628%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
gcp_variant_transforms/libs/vcf_file_composer.py	3	26	11.54%

Files with Coverage Reduction	New Missed Lines	%
gcp_variant_transforms/libs/vcf_file_composer.py	1	22.73%

Totals
Change from base Build 1381:	-0.2%
Covered Lines:	6261
Relevant Lines:	7145

💛 - Coveralls

arostamianfar

Thanks for making this much more robust! Just a few restructuring comments to make it a bit more readable.

arostamianfar · 2018-10-16T20:50:46Z

gcp_variant_transforms/libs/vcf_file_composer.py

 # Cloud Storage allows to compose up to 32 objects.
 _MAX_NUM_OF_BLOBS_PER_COMPOSE = 32
+_NUMBER_OF_API_CALL_RETRIES = 5
+_TIMEOUT = 30


s/_TIMEOUT/_COMPOSE_TIMEOUT_SECONDS
In general, always include the unit in time variables as this can easily be interpreted as 30ms :)

Nice tip! Done.

arostamianfar · 2018-10-17T14:28:13Z

gcp_variant_transforms/libs/vcf_file_composer.py

    new_blob_prefix = filesystems.FileSystems.join(blob_prefix, 'composed_')
-
-    proc_pool = multiprocessing.Pool()
+    arguments = []


s/arguments/blobs_to_compose_args
"arguments" is a very generic name, so it's not clear what it's representing.

Thanks! Done.

arostamianfar · 2018-10-17T14:29:13Z

gcp_variant_transforms/libs/vcf_file_composer.py

+      arguments.append(
+          (self._project, self._bucket_name, blob_names, new_blob_name))
+
+    retry = 0


s/retry/num_retries (retry usually indicates a boolean variable)

arostamianfar · 2018-10-17T14:32:37Z

gcp_variant_transforms/libs/vcf_file_composer.py

+    while arguments:
+      proc_pool = multiprocessing.Pool(processes=8)
+      results = []
+      failed_composing_arguments = []


nit: please move this to just after proc_pool.close() ... in general, it's best to declare variables as closes to where they're first used (we sometimes don't do this for unit tests as it's more readable to group the 'expected' cases together, but we should try to do this in the actual code).
Also, I think we can rename this to failed_blobs_to_compose_args to match the above.

Thanks! Done.

arostamianfar · 2018-10-17T14:58:17Z

gcp_variant_transforms/libs/vcf_file_composer.py


 # Cloud Storage allows to compose up to 32 objects.
 _MAX_NUM_OF_BLOBS_PER_COMPOSE = 32
+_NUMBER_OF_API_CALL_RETRIES = 5


nit: s/_NUMBER_OF_API_CALL_RETRIES/_MAX_NUM_COMPOSE_RETRIES

arostamianfar · 2018-10-17T14:59:41Z

gcp_variant_transforms/libs/vcf_file_composer.py

+                          'timeout.', argument[2][0], argument[2][-1])
+          failed_composing_arguments.append(argument)
+
+      arguments = failed_composing_arguments


I think it's more readable to do:

while num_retries <= _MAX_RETRIES: ... if failed_blobs_to_compose_args: num_retries += 1 blobs_to_compose_args = failed_blobs_to_compose_args logging.warning(...) else: break else: raise RuntimeError(...)

The control flow is more obvious that way (e.g. the while loop clearly defines that it's based on number of retries rather than having arguments; the failed conditions are also more explicitly stated). Note that it uses pythonic 'while-else' statement.
But lemme know what you think or if you have other ideas!

arostamianfar · 2018-10-17T15:11:57Z

gcp_variant_transforms/libs/vcf_file_composer.py

+        else:
+          logging.warning(
+              '%d jobs of composing of blobs failed due to timeout. Retry for '
+              'the %d time.', len(arguments), retry)


nit: s/Retry for ... / Retrying %d of %d times. ... so it becomes, "Retrying 1 of 3 times."

Thanks. Done.

allieychen

Thanks Asha for the review. It seems much cleaner! PTAL :)

allieychen · 2018-10-17T15:49:23Z

gcp_variant_transforms/libs/vcf_file_composer.py


 # Cloud Storage allows to compose up to 32 objects.
 _MAX_NUM_OF_BLOBS_PER_COMPOSE = 32
+_NUMBER_OF_API_CALL_RETRIES = 5


allieychen · 2018-10-17T15:49:36Z

gcp_variant_transforms/libs/vcf_file_composer.py

 # Cloud Storage allows to compose up to 32 objects.
 _MAX_NUM_OF_BLOBS_PER_COMPOSE = 32
+_NUMBER_OF_API_CALL_RETRIES = 5
+_TIMEOUT = 30


Nice tip! Done.

allieychen · 2018-10-17T15:51:20Z

gcp_variant_transforms/libs/vcf_file_composer.py

    new_blob_prefix = filesystems.FileSystems.join(blob_prefix, 'composed_')
-
-    proc_pool = multiprocessing.Pool()
+    arguments = []


Thanks! Done.

allieychen · 2018-10-17T15:52:11Z

gcp_variant_transforms/libs/vcf_file_composer.py

+      arguments.append(
+          (self._project, self._bucket_name, blob_names, new_blob_name))
+
+    retry = 0


allieychen · 2018-10-17T15:55:13Z

gcp_variant_transforms/libs/vcf_file_composer.py

+    while arguments:
+      proc_pool = multiprocessing.Pool(processes=8)
+      results = []
+      failed_composing_arguments = []


Thanks! Done.

allieychen · 2018-10-17T16:57:28Z

gcp_variant_transforms/libs/vcf_file_composer.py

+        else:
+          logging.warning(
+              '%d jobs of composing of blobs failed due to timeout. Retry for '
+              'the %d time.', len(arguments), retry)


Thanks. Done.

arostamianfar

LGTM! just two nits...

arostamianfar · 2018-10-17T17:43:31Z

gcp_variant_transforms/libs/vcf_file_composer.py

+      else:
+        break
+    else:
+      raise RuntimeError('Composing of blobs fails after {} '


nit: s/fails/failed

arostamianfar · 2018-10-17T17:44:21Z

gcp_variant_transforms/libs/vcf_file_composer.py

+        num_retries += 1
+        blobs_to_compose_args = failed_blobs_to_compose_args
+        logging.warning(
+            '%d jobs of composing of blobs failed due to timeout. Retrying for '


nit: just "Retrying %d of %d." is cleaner (I think it should be "times" as "time" has a different meaning, but we can just remove it :) ).

allieychen requested a review from arostamianfar October 16, 2018 18:12

arostamianfar reviewed Oct 17, 2018

View reviewed changes

allieychen added 2 commits October 17, 2018 13:15

retry on failure

696bcb4

address the comments

1dba862

allieychen force-pushed the fix branch from 734fde7 to 1dba862 Compare October 17, 2018 17:16

allieychen commented Oct 17, 2018

View reviewed changes

arostamianfar approved these changes Oct 17, 2018

View reviewed changes

address the comments

1b9f15c

allieychen merged commit 11f7d44 into googlegenomics:master Oct 17, 2018

allieychen deleted the fix branch October 17, 2018 18:16

retry on failure #381

retry on failure #381

Uh oh!

Conversation

allieychen commented Oct 16, 2018

Uh oh!

coveralls commented Oct 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1384

💛 - Coveralls

Uh oh!

arostamianfar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allieychen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arostamianfar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Oct 17, 2018 •

edited

Loading