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

Make AutomationCondition evaluate optionally async #25238

Merged
merged 5 commits into from
Oct 22, 2024
Merged

Make AutomationCondition evaluate optionally async #25238

merged 5 commits into from
Oct 22, 2024

Conversation

briantu
Copy link
Contributor

@briantu briantu commented Oct 11, 2024 •
edited
Loading

Summary & Motivation

We now want to make each the AutomationCondition.evaluate function optionally async. This is because we still want to keep it non async if users want to make their own automation conditions. To handle this, added a wrapper async evaluate function on AutomationContext that checks if the evaluate function is async or not.

How I Tested These Changes

Existing tests should pass

@briantu briantu requested a review from OwenKephart October 11, 2024 20:58
@briantu briantu marked this pull request as ready for review October 11, 2024 20:59
@briantu Graphite App
Copy link
Contributor Author

briantu commented Oct 11, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @briantu and the rest of your teammates on Graphite Graphite

@briantu briantu mentioned this pull request Oct 11, 2024
Merged
@briantu briantu force-pushed the briantu/make-da-evaluation-optionally-async branch from 485dccf to 89abd65 Compare October 11, 2024 21:06
@briantu briantu requested a review from OwenKephart October 11, 2024 21:30
OwenKephart
OwenKephart approved these changes Oct 11, 2024
View reviewed changes
Copy link
Contributor

@OwenKephart OwenKephart left a comment

Choose a reason for hiding this comment

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

note comment, otherwise this is looking great so far!

...modules/dagster/dagster/_core/definitions/declarative_automation/operators/since_operator.py Outdated Show resolved Hide resolved
@briantu briantu force-pushed the briantu/make-da-evaluation-optionally-async branch from 819f593 to ebe8594 Compare October 14, 2024 19:06
@briantu briantu force-pushed the briantu/evaluate-automation-condition-async branch from 26284dd to 6a70785 Compare October 14, 2024 19:06
This was referenced Oct 14, 2024
Merged
Merged
Closed
@dagster-io dagster-io deleted a comment from github-actions bot Oct 15, 2024
@dagster-io dagster-io deleted a comment from github-actions bot Oct 15, 2024
@dagster-io dagster-io deleted a comment from github-actions bot Oct 15, 2024
@dagster-io dagster-io deleted a comment from github-actions bot Oct 15, 2024
@dagster-io dagster-io deleted a comment from github-actions bot Oct 15, 2024
@briantu briantu force-pushed the briantu/make-da-evaluation-optionally-async branch 2 times, most recently from 34a2997 to 73818b7 Compare October 15, 2024 22:35
@briantu briantu force-pushed the briantu/evaluate-automation-condition-async branch from 3a3fce1 to 4ed4f5e Compare October 15, 2024 22:42
@briantu briantu force-pushed the briantu/make-da-evaluation-optionally-async branch from 73818b7 to d56c073 Compare October 15, 2024 22:42
@briantu briantu force-pushed the briantu/evaluate-automation-condition-async branch from 4ed4f5e to 5072f1c Compare October 17, 2024 19:00
@briantu briantu force-pushed the briantu/make-da-evaluation-optionally-async branch from d56c073 to def9fe2 Compare October 17, 2024 19:01
@briantu briantu force-pushed the briantu/evaluate-automation-condition-async branch from 5072f1c to c184505 Compare October 17, 2024 20:36
@briantu briantu force-pushed the briantu/make-da-evaluation-optionally-async branch 2 times, most recently from 4a164ec to 7fc9c05 Compare October 18, 2024 17:59
@briantu briantu mentioned this pull request Oct 18, 2024
Merged
@briantu briantu force-pushed the briantu/evaluate-automation-condition-async branch from 110b6da to f929335 Compare October 18, 2024 22:49
@briantu briantu force-pushed the briantu/make-da-evaluation-optionally-async branch from 7fc9c05 to 0f48def Compare October 18, 2024 22:49
@briantu briantu force-pushed the briantu/evaluate-automation-condition-async branch from ce5bef1 to c63f9ec Compare October 21, 2024 16:41
@briantu briantu force-pushed the briantu/make-da-evaluation-optionally-async branch 2 times, most recently from 7bf9c80 to 81cf176 Compare October 21, 2024 19:09
@briantu briantu force-pushed the briantu/evaluate-automation-condition-async branch from c63f9ec to beea1b4 Compare October 21, 2024 19:52
@briantu briantu force-pushed the briantu/make-da-evaluation-optionally-async branch from 81cf176 to 30253d1 Compare October 21, 2024 19:52
@briantu briantu force-pushed the briantu/evaluate-automation-condition-async branch from beea1b4 to de5672e Compare October 21, 2024 22:00
@briantu briantu force-pushed the briantu/make-da-evaluation-optionally-async branch from 30253d1 to 8bdb5e8 Compare October 21, 2024 22:00
@briantu briantu force-pushed the briantu/evaluate-automation-condition-async branch from de5672e to 7469355 Compare October 22, 2024 00:45
@briantu briantu force-pushed the briantu/make-da-evaluation-optionally-async branch from 8bdb5e8 to 0d4e8b4 Compare October 22, 2024 00:45
@briantu briantu force-pushed the briantu/evaluate-automation-condition-async branch from 2e2bbcd to 047f4eb Compare October 22, 2024 19:39
@briantu briantu force-pushed the briantu/make-da-evaluation-optionally-async branch from 0d4e8b4 to c957043 Compare October 22, 2024 19:39
@briantu briantu force-pushed the briantu/evaluate-automation-condition-async branch from 047f4eb to c0e6562 Compare October 22, 2024 20:24
@briantu briantu force-pushed the briantu/make-da-evaluation-optionally-async branch from c957043 to 346ee2b Compare October 22, 2024 20:24
@briantu briantu force-pushed the briantu/evaluate-automation-condition-async branch from c0e6562 to 57bebf0 Compare October 22, 2024 20:30
@briantu briantu force-pushed the briantu/make-da-evaluation-optionally-async branch from 346ee2b to b115f84 Compare October 22, 2024 20:31
@briantu briantu force-pushed the briantu/evaluate-automation-condition-async branch from 57bebf0 to 1c5c461 Compare October 22, 2024 21:13
@briantu briantu force-pushed the briantu/make-da-evaluation-optionally-async branch from b115f84 to 7e354ee Compare October 22, 2024 21:13
@briantu Graphite App
Copy link
Contributor Author

briantu commented Oct 22, 2024 •
edited
Loading

Merge activity

  • Oct 22, 6:08 PM EDT: A user started a stack merge that includes this pull request via Graphite.
  • Oct 22, 6:18 PM EDT: Graphite rebased this pull request as part of a merge.
  • Oct 22, 6:19 PM EDT: A user merged this pull request with Graphite.

@briantu briantu changed the base branch from briantu/evaluate-automation-condition-async to graphite-base/25238 October 22, 2024 22:13
@briantu briantu changed the base branch from graphite-base/25238 to master October 22, 2024 22:15
@briantu briantu force-pushed the briantu/make-da-evaluation-optionally-async branch from 7e354ee to a528dbc Compare October 22, 2024 22:17
@briantu briantu merged commit ea8bdb2 into master Oct 22, 2024
1 check failed
@briantu briantu deleted the briantu/make-da-evaluation-optionally-async branch October 22, 2024 22:19
Grzyblon pushed a commit to Grzyblon/dagster that referenced this pull request Oct 26, 2024
@briantu @Grzyblon
Make AutomationCondition evaluate optionally async (dagster-io#25238)
9e39cfa 
## Summary & Motivation
We now want to make each the `AutomationCondition.evaluate` function optionally async. This is because we still want to keep it non async if users want to make their own automation conditions. To handle this, added a wrapper async evaluate function on `AutomationContext` that checks if the evaluate function is async or not.

## How I Tested These Changes
Existing tests should pass
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OwenKephart OwenKephart OwenKephart approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@briantu @OwenKephart