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.

Sign up for GitHub

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

Jump to bottom

Allow GCP global pipeline timeout to be configurable #5273

Merged
merged 4 commits into from
Jan 21, 2020
Merged

Allow GCP global pipeline timeout to be configurable #5273

merged 4 commits into from
Jan 21, 2020

Conversation

hkeward
Copy link
Contributor

@hkeward hkeward commented Nov 9, 2019

If a timeout is not provided, GCP defaults to setting a timeout of 7 days, after which the pipeline will abort.

Occasionally pipelines genuinely need to run >7 days. These changes allow this value to be user-configured.

@cjllanwarne cjllanwarne added the Community Contribution A pull request from the Cromwell open-source community label Nov 12, 2019
cjllanwarne
cjllanwarne reviewed Nov 18, 2019
View reviewed changes
Copy link
Contributor

@cjllanwarne cjllanwarne left a comment

Choose a reason for hiding this comment

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

Have you tested that this is respected if given more than 7 days?

It'd be a shame for this PR if Google expected that people might want to lower this value, but didn't want anyone to raise it.

cromwell.example.backends/PAPIv2.conf Outdated Show resolved Hide resolved
...ain/scala/cromwell/backend/google/pipelines/common/PipelinesApiConfigurationAttributes.scala Outdated Show resolved Hide resolved
...nes/v2alpha1/src/main/scala/cromwell/backend/google/pipelines/v2alpha1/GenomicsFactory.scala Outdated Show resolved Hide resolved
@aednichols
Copy link
Collaborator

It'd be a shame for this PR if Google expected that people might want to lower this value, but didn't want anyone to raise it.

Google confirmed to me that yes, it can be raised, up to 30 days – that might be a useful thing to note in a config comment.

See https://broadworkbench.atlassian.net/browse/BA-6015

@aednichols
Copy link
Collaborator

@hkeward welcome to our repo and thank you for your contribution. We were actually looking at making this timeout configurable ourselves, so you've done some of our work for us.

@patmagee
Copy link

@cjllanwarne FYI @hkeward and I encountered this while running some Structural variant calling using delly. Its an edge case that we have encountered maybe once before, but its great to see that you guys were thinking of implementing it already.

@aednichols is that a hard cap on 30 day? In that case, it might be advisable to check the maximum value and strictly require less 30 days

@aednichols
Copy link
Collaborator

Yes @patmagee hard cap

@aednichols
Copy link
Collaborator

Hi @hkeward, would you mind updating your branch from our latest develop? We have some important test reliability fixes that need picking up.

mcovarr
mcovarr reviewed Jan 15, 2020
View reviewed changes
Copy link
Contributor

@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 very handy config option like this should be documented in docs/Google.md and also perhaps CHANGELOG.md

...ain/scala/cromwell/backend/google/pipelines/common/PipelinesApiConfigurationAttributes.scala Outdated Show resolved Hide resolved
mcovarr
mcovarr approved these changes Jan 21, 2020
View reviewed changes
Copy link
Contributor

@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.

Awesome, thanks again for this contribution! 😄

@aednichols
Copy link
Collaborator

Passed CI at #5376 with same commit ddbd163, merging

@aednichols aednichols merged commit 8671826 into broadinstitute:develop Jan 21, 2020
@hkeward hkeward deleted the gcp_allow_timeout_configuration branch January 30, 2020 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjllanwarne cjllanwarne cjllanwarne left review comments

@mcovarr mcovarr mcovarr approved these changes

@aednichols aednichols aednichols approved these changes

Assignees
No one assigned
Labels
Community Contribution A pull request from the Cromwell open-source community
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hkeward @aednichols @patmagee @mcovarr @cjllanwarne