[amp-refactor][15/n] Rename AssetConditionCursor -> AssetConditionEvaluationState #19035

OwenKephart · 2024-01-04T22:19:37Z

Summary & Motivation

After some deliberation, "cursor" seems to be a poor name for what these objects are actually tracking. Instead, we go with AssetConditionEvaluationInfo as a more general name.

We do still want to distinguish this object, which is used to index/checkpoint computation, from the AssetConditionEvaluationResult object, which contains only a subset of the total information (related to what was actually calculated on that tick)

This PR also removes the unnecessary AssetConditionEvaluationResult object that used to exist as an in-memory-only organizational tool, in light of the fact that we can just directly return AssetConditionEvaluationInfo objects from the body of the evaluate function.

How I Tested These Changes

OwenKephart · 2024-01-04T22:20:05Z

Current dependencies on/for this PR:

master
- PR [amp-refactor][1/n] Update context objects #18612
  - PR [amp-refactor][2/n] Create snapshot class for condition objects #18613
    - PR [amp-refactor][3/n] RuleEvaluationData -> SubsetWithMetadata #18614
      - PR [amp-refactor][4/n] AssetAutomationCondition -> AssetCondition #18615
        
        PR [amp-refactor][5/n] Whitelist AssetConditionEvaluation #18656
        
        PR [amp-refactor][6/n] Simplify AssetConditionEvaluationContext #18684
        
        PR [amp-refactor][7/n] Refactor AssetDaemonAssetCursor -> AssetConditionCursor #18953
        
        PR [amp-refactor][8/n] Whitelist AssetDaemonCursor for serdes #18954
        
        PR [amp-refactor][9/n] Keep an accurate per-asset cursor #18775
        PR [amp-refactor][10/n] Improve backcompat logic #18871
        PR [amp-refactor][11/n] Create AssetConditionEvaluationResult object, remove AssetConditionCursorExtras #18804
        PR [amp-refactor][12/n] Return AssetConditionEvaluationResult from AutoMaterializeRule #18823
        PR [amp-refactor][13/n] Add start/end timestamps to evaluations #18956
        PR [amp-refactor][14/n] Fix non-subsettable multi-asset handling #19012
        PR [amp-refactor][15/n] Rename AssetConditionCursor -> AssetConditionEvaluationState #19035 👈
        PR [amp-refactor][16/n] Re-enable legacy GQL queries #19081
        PR [amp-refactor][17/n] AssetConditionEvaluation GQL #18708
        
        PR [amp-refactor][?/n] Add asset_condition argument to AssetsDefinition and friends #18802
        
        PR [amp-refactor][OBSOLETE] #18693
        
        PR [amp-refactor][OBSOLETE] #18719

This stack of pull requests is managed by Graphite.

github-actions · 2024-01-08T20:50:11Z

Deploy preview for dagit-storybook ready!

✅ Preview
https://dagit-storybook-4ug7zbmak-elementl.vercel.app
https://01-04-Rename-AssetConditionCursor.components-storybook.dagster-docs.io

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

github-actions · 2024-01-08T20:50:45Z

Deploy preview for dagster-university ready!

✅ Preview
https://dagster-university-h41vw8l2k-elementl.vercel.app
https://01-04-Rename-AssetConditionCursor.dagster-university.dagster-docs.io

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

sryza

We settled on different names in this thread, right? https://dagsterlabs.slack.com/archives/C04UD72HHQX/p1704326165574669

OwenKephart · 2024-01-17T00:51:08Z

@sryza renamed/refactored to align w/ that thread

sryza

A few final comments / questions, otherwise looks good to go

sryza · 2024-01-17T16:06:51Z

python_modules/dagster/dagster/_core/definitions/asset_condition.py

    def equivalent_to_stored_evaluation(self, other: Optional["AssetConditionEvaluation"]) -> bool:
-        """Returns if all fields other than `run_ids` are equal."""
+        """Returns if this evaluation is functionally equivalent to the given stored evaluation.


What are the situations where they would be considered functionality equivalent but not __equal__? Worth mentioning at least an example here.

sryza · 2024-01-17T16:07:40Z

python_modules/dagster/dagster/_core/definitions/asset_condition.py

    evaluation: AssetConditionEvaluation
-    extra_values_by_unique_id: Mapping[str, PackableValue]
+    evaluation_timestamp: Optional[float]


Is this redundant with the timestamp on evaluation?

Not exactly. The "evaluation_timestamp" is used as the explicit timestamp for the entire tick's evaluation. This is synchronized across all assets (in theory, this means that you can't get weird situations where we evaluate right at an hour boundary and so two assets with identical partitions definitions were interpreted as having a different set of partitions if one was evaluated before and the other after the hour transition).

This timestamp is used for things like cron schedule ticks as well.

In contrast, the "start_timestamp" on the root evaluation is just when we started evaluating that particular asset (so we can calculate how long that evaluation took).

Ah, got it. Would it make sense to call it previous_tick_evaluation_timestamp to disambiguate?

python_modules/dagster/dagster/_core/definitions/asset_condition.py

sryza · 2024-01-17T16:12:11Z

python_modules/dagster/dagster/_core/definitions/asset_condition.py


-    condition: "AssetCondition"
    evaluation: AssetConditionEvaluation


Thoughts on calling this latest_evaluation (with implications for some of the other properties)?

Yeah I've been going back and forth on that point -- my feeling was that it might be redundant, as we can just call the entire object previous_evaluation_state. If we do that, then previous_evaluation_state.previous_evaluation is a bit weird.

The way I am inclined to look at it is that the "state" isn't exactly the state of the previous valuation. It's more that the ongoing incremental state we track includes the previous evaluation.

E.g. imagine a world where we want to get smart about the order that we compute rules in, and, to do that, we want to track statistics on the typical amount of time it takes to evaluate conditions. To avoid overfitting on the previous tick, we might want to track the last N ticks. That data could make sense to store inside this object, but wouldn't be part of the previous evaluation.

ok updated it to previous_evaluation and previous_tick_evaluation_timestamp. kept "max_storage_id" as is, as "previous_max_storage_id" feels like the max storage id that was used for the previous evaluation, which is one level "too deep"

OwenKephart · 2024-01-19T21:20:30Z

Merge activity

Jan 19, 4:20 PM: @OwenKephart started a stack merge that includes this pull request via Graphite.
Jan 19, 4:47 PM: Graphite rebased this pull request as part of a merge.
Jan 19, 4:48 PM: @OwenKephart merged this pull request with Graphite.

…luationState (#19035) ## Summary & Motivation After some deliberation, "cursor" seems to be a poor name for what these objects are actually tracking. Instead, we go with AssetConditionEvaluationInfo as a more general name. We do still want to distinguish this object, which is used to index/checkpoint computation, from the AssetConditionEvaluationResult object, which contains only a subset of the total information (related to what was actually calculated on that tick) This PR also removes the unnecessary AssetConditionEvaluationResult object that used to exist as an in-memory-only organizational tool, in light of the fact that we can just directly return AssetConditionEvaluationInfo objects from the body of the evaluate function. ## How I Tested These Changes

OwenKephart mentioned this pull request Jan 4, 2024

[amp-refactor][14/n] Fix non-subsettable multi-asset handling #19012

Merged

OwenKephart force-pushed the 01-03-Fix_non-subsettable_multi-asset_handling branch from f08149c to 7c58a41 Compare January 5, 2024 16:42

OwenKephart force-pushed the 01-04-Rename_AssetConditionCursor branch from a4ccc5e to f7ef51e Compare January 5, 2024 16:42

OwenKephart mentioned this pull request Jan 8, 2024

[amp-refactor][16/n] Re-enable legacy GQL queries #19081

Merged

OwenKephart force-pushed the 01-04-Rename_AssetConditionCursor branch from f7ef51e to 0bd84b0 Compare January 8, 2024 20:40

OwenKephart force-pushed the 01-03-Fix_non-subsettable_multi-asset_handling branch from 7c58a41 to 4583f25 Compare January 8, 2024 20:40

OwenKephart force-pushed the 01-03-Fix_non-subsettable_multi-asset_handling branch from 4583f25 to dd4086f Compare January 9, 2024 17:08

OwenKephart force-pushed the 01-04-Rename_AssetConditionCursor branch from 0bd84b0 to 26af907 Compare January 9, 2024 17:08

OwenKephart force-pushed the 01-03-Fix_non-subsettable_multi-asset_handling branch from dd4086f to 18d1b83 Compare January 11, 2024 22:21

OwenKephart force-pushed the 01-04-Rename_AssetConditionCursor branch from 26af907 to 0313f70 Compare January 11, 2024 22:22

OwenKephart force-pushed the 01-04-Rename_AssetConditionCursor branch from e98b265 to 14d39a6 Compare January 12, 2024 18:02

OwenKephart force-pushed the 01-03-Fix_non-subsettable_multi-asset_handling branch from 42b91bc to 309325a Compare January 12, 2024 22:12

OwenKephart force-pushed the 01-04-Rename_AssetConditionCursor branch from 14d39a6 to 953dffd Compare January 12, 2024 22:12

OwenKephart changed the title ~~[amp-refactor][?/n] Rename AssetConditionCursor -> AssetConditionEvaluationInfo~~ [amp-refactor][15/n] Rename AssetConditionCursor -> AssetConditionEvaluationInfo Jan 12, 2024

OwenKephart requested a review from sryza January 12, 2024 23:41

OwenKephart marked this pull request as ready for review January 12, 2024 23:41

sryza reviewed Jan 12, 2024

View reviewed changes

OwenKephart force-pushed the 01-03-Fix_non-subsettable_multi-asset_handling branch from 309325a to 1bb1419 Compare January 16, 2024 22:25

OwenKephart force-pushed the 01-04-Rename_AssetConditionCursor branch 2 times, most recently from 674bc95 to cb8d03b Compare January 17, 2024 00:49

OwenKephart requested a review from sryza January 17, 2024 00:50

OwenKephart force-pushed the 01-04-Rename_AssetConditionCursor branch from cb8d03b to 7add39d Compare January 17, 2024 01:18

sryza reviewed Jan 17, 2024

View reviewed changes

OwenKephart force-pushed the 01-04-Rename_AssetConditionCursor branch from 7add39d to 60e1c31 Compare January 17, 2024 17:29

OwenKephart changed the title ~~[amp-refactor][15/n] Rename AssetConditionCursor -> AssetConditionEvaluationInfo~~ [amp-refactor][15/n] Rename AssetConditionCursor -> AssetConditionEvaluationState Jan 17, 2024

OwenKephart requested a review from sryza January 17, 2024 17:50

OwenKephart force-pushed the 01-03-Fix_non-subsettable_multi-asset_handling branch from 1bb1419 to b1a03c9 Compare January 17, 2024 23:11

OwenKephart force-pushed the 01-04-Rename_AssetConditionCursor branch from 60e1c31 to d27f79a Compare January 17, 2024 23:11

OwenKephart force-pushed the 01-03-Fix_non-subsettable_multi-asset_handling branch from b1a03c9 to c068ebc Compare January 18, 2024 23:17

OwenKephart force-pushed the 01-04-Rename_AssetConditionCursor branch from d27f79a to d0f2f5b Compare January 18, 2024 23:17

sryza approved these changes Jan 18, 2024

View reviewed changes

OwenKephart force-pushed the 01-03-Fix_non-subsettable_multi-asset_handling branch from c068ebc to d08f558 Compare January 19, 2024 21:44

Base automatically changed from 01-03-Fix_non-subsettable_multi-asset_handling to master January 19, 2024 21:46

Rename AssetConditionCursor

4e26389

OwenKephart force-pushed the 01-04-Rename_AssetConditionCursor branch from d0f2f5b to 4e26389 Compare January 19, 2024 21:47

OwenKephart merged commit 1e15fef into master Jan 19, 2024
1 check was pending

OwenKephart deleted the 01-04-Rename_AssetConditionCursor branch January 19, 2024 21:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[amp-refactor][15/n] Rename AssetConditionCursor -> AssetConditionEvaluationState #19035

[amp-refactor][15/n] Rename AssetConditionCursor -> AssetConditionEvaluationState #19035

OwenKephart commented Jan 4, 2024

OwenKephart commented Jan 4, 2024 •

edited

Loading

github-actions bot commented Jan 8, 2024

github-actions bot commented Jan 8, 2024

sryza left a comment

OwenKephart commented Jan 17, 2024

sryza left a comment

sryza Jan 17, 2024

OwenKephart Jan 18, 2024

sryza Jan 17, 2024

OwenKephart Jan 17, 2024

sryza Jan 17, 2024

OwenKephart Jan 18, 2024

sryza Jan 17, 2024

OwenKephart Jan 17, 2024

sryza Jan 17, 2024

OwenKephart Jan 18, 2024

OwenKephart commented Jan 19, 2024 •

edited

Loading


		condition: "AssetCondition"
		evaluation: AssetConditionEvaluation

[amp-refactor][15/n] Rename AssetConditionCursor -> AssetConditionEvaluationState #19035

[amp-refactor][15/n] Rename AssetConditionCursor -> AssetConditionEvaluationState #19035

Conversation

OwenKephart commented Jan 4, 2024

Summary & Motivation

How I Tested These Changes

OwenKephart commented Jan 4, 2024 • edited Loading

github-actions bot commented Jan 8, 2024

github-actions bot commented Jan 8, 2024

sryza left a comment

Choose a reason for hiding this comment

OwenKephart commented Jan 17, 2024

sryza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OwenKephart commented Jan 19, 2024 • edited Loading

Merge activity

OwenKephart commented Jan 4, 2024 •

edited

Loading

OwenKephart commented Jan 19, 2024 •

edited

Loading