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

LoadData maxRetries parameterized, default increased [VS-383] #7791

Merged
merged 2 commits into from
Apr 20, 2022

Conversation

mcovarr
Copy link
Collaborator

@mcovarr mcovarr commented Apr 19, 2022

This task was already eligible for retry but only once, this changes the default retries to 3 and makes it an optional workflow input parameter. Incremental improvement toward VS-261.

@mcovarr mcovarr requested review from rsasch and gbggrant April 19, 2022 12:22
@gatk-bot
Copy link

Travis reported job failures from build 38945
Failures in the following jobs:

Test Type JDK Job ID Logs
conda openjdk8 38945.5 logs

Copy link
Collaborator

@gbggrant gbggrant left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -204,6 +206,7 @@ task LoadData {

File? gatk_override
Int? load_data_preemptible_override
Int? load_data_maxretries_override
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have rule for why this would be optional?
You could write it as Int load_data_maxretries_override = 3
and then you wouldn't need the select_first in the runtime block

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't have a strong opinion on this, I was following the existing pattern used for load_data_preemptible_override. Happy to discuss best practices!

Copy link
Contributor

Choose a reason for hiding this comment

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

yes and I think we were using GATK patterns, but it does seem superfluous to me

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we think there would ever be a reason to retry with more mem? If yes I suppose we should add it to the Terra Quickstart

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My very limited experience is exclusively with the Quickstart and I don't think that would have benefited from more memory here, but if you're seeing something different with more "real" data sets we could certainly add that.

@mcovarr mcovarr merged commit 940777c into ah_var_store Apr 20, 2022
@mcovarr mcovarr deleted the vs_383_load_data_improvements branch April 20, 2022 16:37
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.

4 participants