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

Expose deadline argument for DAG #678

Merged
merged 2 commits into from
Nov 15, 2024
Merged

Expose deadline argument for DAG #678

merged 2 commits into from
Nov 15, 2024

Conversation

ktsitsi
Copy link
Collaborator

@ktsitsi ktsitsi commented Nov 7, 2024

This PR:

  • Exposes the deadline argument for DAG timeouts to the bioimg ingestion API.

@ktsitsi ktsitsi force-pushed the kt/sc-59046/expose-deadline-argument-in-bioimg-ingestion-tg branch from e7d6ae5 to e2fe76f Compare November 7, 2024 16:27
sgillies
sgillies approved these changes Nov 7, 2024
View reviewed changes
Copy link
Collaborator

@sgillies sgillies left a comment

Choose a reason for hiding this comment

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

@ktsitsi do we need to add any documentation about the expected state of an ingest when it times out? Or whether a user needs to take any cleanup actions?

@spencerseale
Copy link
Collaborator

@sgillies no cleanup will be needed thanks to validation steps @ktsitsi has put in place. More docs the better

@spencerseale
Copy link
Collaborator

@ktsitsi can we include the default rather than None? I know None will result in using REST's timeout which is 24 hours, but timeout: int = 86400 would make it more obvious. We could also say in the doc str the default is 24 hours, but that would need to stay in sync with any potential changes in the REST config.

Users will want to know what the default timeout is I think

@sgillies sgillies self-assigned this Nov 15, 2024
@sgillies
Copy link
Collaborator

Failures are unrelated (and addressed in main). Merging.

@sgillies sgillies merged commit 9817329 into main Nov 15, 2024
16 of 17 checks passed
@sgillies sgillies deleted the kt/sc-59046/expose-deadline-argument-in-bioimg-ingestion-tg branch November 15, 2024 15:20
spencerseale pushed a commit that referenced this pull request Dec 17, 2024
@ktsitsi @sgillies @spencerseale
Expose deadline argument for DAG (#678)
d49ed9f 
* Expose deadline argument for DAG

* Use 86400 as default instead of None

---------

Co-authored-by: Sean Gillies <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Shelnutt2 Shelnutt2 Shelnutt2 left review comments

@sgillies sgillies sgillies approved these changes

@spencerseale spencerseale Awaiting requested review from spencerseale

@ihnorton ihnorton Awaiting requested review from ihnorton

Assignees

@sgillies sgillies

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ktsitsi @spencerseale @sgillies @Shelnutt2