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

[rfc] Add client timeout for schedules/sensors #17831

Merged
merged 1 commit into from
Nov 30, 2023
Merged

[rfc] Add client timeout for schedules/sensors #17831

merged 1 commit into from
Nov 30, 2023

Conversation

dpeng817
Copy link
Contributor

@dpeng817 dpeng817 commented Nov 8, 2023 •
edited
Loading

Allows timeout retrieval from the execution args for schedules/sensors.

This provides a pathway for providing per-sensor timeouts, or in general, specifying timeout at the client.

@dpeng817
Copy link
Contributor Author

dpeng817 commented Nov 8, 2023 •
edited
Loading

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@dpeng817 dpeng817 mentioned this pull request Nov 8, 2023
Merged
@dpeng817 dpeng817 marked this pull request as ready for review November 8, 2023 21:56
@dpeng817 dpeng817 changed the title Add client timeout for schedules/sensors [rfc] Add client timeout for schedules/sensors Nov 8, 2023
@dpeng817 dpeng817 force-pushed the dpeng817/add_client_timeout branch from 16a2eee to 14e47d0 Compare November 8, 2023 22:37
rexledesma
rexledesma reviewed Nov 8, 2023
View reviewed changes
Copy link
Contributor

@rexledesma rexledesma left a comment

Choose a reason for hiding this comment

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

Left a comment on min/max of timeouts

python_modules/dagster/dagster/_grpc/client.py Outdated
# 3. By the client.
# The timeout argument of this function takes into account (1) and (2), while
# the client may pass a timeout argument via the
# `external_schedule_execution_args` object. We take the maximum of all of
Copy link
Contributor

@rexledesma rexledesma Nov 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I would assume that the timeout would be sensitive, in that stricter timeout (e.g. coming from the client) should always be respected, even if the server timeout is higher.

So shouldn't we be taking the minimum of all these timeouts?

(I may be confusing this setting with our previous conversation today about setting per-sensor timeouts)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The goal here is to allow the client to set a higher timeout, in cases where the env var cannot be set. If we take the minimum, then we'll always end up using the current 60s window provided by the grpc call itself at most.
Perhaps an alternative is to, instead of taking the max or min, always defer to the value set on the client if provided, and then otherwise use the default. That way, max or min will be respected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to always defer to the timeout set on the client, which should allow for tighter client timeouts as well.

@dpeng817 dpeng817 force-pushed the dpeng817/add_client_timeout branch from 14e47d0 to 92f5396 Compare November 8, 2023 23:58
@dpeng817 dpeng817 requested a review from rexledesma November 9, 2023 00:14
@dpeng817 dpeng817 mentioned this pull request Nov 9, 2023
Closed
gibsondan
gibsondan previously requested changes Nov 10, 2023
View reviewed changes
Copy link
Member

@gibsondan gibsondan left a comment

Choose a reason for hiding this comment

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

this seems ok to me with the addition of tests (plus the change suggested inline where we take out the unused argument to the function)

python_modules/dagster/dagster/_grpc/client.py Outdated Show resolved Hide resolved
@dpeng817 dpeng817 force-pushed the dpeng817/add_client_timeout branch from 92f5396 to 005642a Compare November 10, 2023 23:27
@dpeng817 dpeng817 requested a review from gibsondan November 10, 2023 23:27
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 10, 2023 •
edited
Loading

Deploy preview for dagit-storybook ready!

✅ Preview
https://dagit-storybook-4iazes8tf-elementl.vercel.app
https://dpeng817-add-client-timeout.components-storybook.dagster-docs.io

Built with commit 11aa374.
This pull request is being automatically deployed with vercel-action

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 10, 2023 •
edited
Loading

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-mn06utm2v-elementl.vercel.app
https://dpeng817-add-client-timeout.core-storybook.dagster-docs.io

Built with commit ab8c7db.
This pull request is being automatically deployed with vercel-action

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 10, 2023 •
edited
Loading

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-1ixa4fno0-elementl.vercel.app
https://dpeng817-add-client-timeout.dagster.dagster-docs.io

Direct link to changed pages:

@dpeng817 dpeng817 force-pushed the dpeng817/add_client_timeout branch from 005642a to aef3b6a Compare November 13, 2023 17:20
gibsondan
gibsondan approved these changes Nov 17, 2023
View reviewed changes
Copy link
Member

@gibsondan gibsondan left a comment

Choose a reason for hiding this comment

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

can we also add something that tests the case where you have both the env var set and the client setting set and the env var is ignored?

python_modules/dagster/dagster/_grpc/client.py
Comment on lines +385 to +389
timeout = (
external_schedule_execution_args.timeout
if external_schedule_execution_args.timeout is not None
else DEFAULT_SCHEDULE_GRPC_TIMEOUT
)
Copy link
Member

Choose a reason for hiding this comment

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

overriding if it's explicitly set seem reasonable to me - we will want to avoid changing the default behavior for folks who have the env var set - so as long as its something you explicitly opt into and we don't, for example, set the default client setting to 60 for everybody and potentially break folks who have the env var set

@dpeng817 dpeng817 force-pushed the dpeng817/add_client_timeout branch from aef3b6a to 8ba53e8 Compare November 22, 2023 23:32
@github-actions GitHub Actions
Copy link

Deploy preview for dagster-university ready!

✅ Preview
https://dagster-university-citfg560g-elementl.vercel.app
https://dpeng817-add-client-timeout.dagster-university.dagster-docs.io

Built with commit 8ba53e8.
This pull request is being automatically deployed with vercel-action

@dpeng817 dpeng817 force-pushed the dpeng817/add_client_timeout branch from 8ba53e8 to 11aa374 Compare November 28, 2023 17:23
@dpeng817 dpeng817 force-pushed the dpeng817/add_client_timeout branch from 11aa374 to ab8c7db Compare November 29, 2023 19:34
@dpeng817 dpeng817 merged commit 4addfa5 into master Nov 30, 2023
3 checks passed
@dpeng817 dpeng817 deleted the dpeng817/add_client_timeout branch November 30, 2023 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibsondan gibsondan gibsondan approved these changes

@rexledesma rexledesma Awaiting requested review from rexledesma

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dpeng817 @gibsondan @rexledesma