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 AssetStatusCacheValue inherit InstanceLoadableBy #25200

Merged
merged 3 commits into from
Oct 22, 2024
Merged

Make AssetStatusCacheValue inherit InstanceLoadableBy #25200

merged 3 commits into from
Oct 22, 2024

Conversation

briantu
Copy link
Contributor

@briantu briantu commented Oct 10, 2024 •
edited
Loading

Summary & Motivation

We want to make AssetStatusCacheValue follow the dataloader pattern as this will help optimize DA evaluation. This PR just uses AssetStatusCacheValue.blocking_get(), but will switch to using AssetStatusCacheValue.gen() when making things async in a follow up PR.

How I Tested These Changes

Existing tests should pass

@briantu Graphite App
Copy link
Contributor Author

briantu commented Oct 10, 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 requested a review from OwenKephart October 10, 2024 20:56
@briantu briantu marked this pull request as ready for review October 10, 2024 20:56
OwenKephart
OwenKephart reviewed Oct 10, 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.

In terms of testing, one thing I'd like to see is what happens to test_perf.py if you add in that snippet of code to report materializations for everything requested in the first tick.

I actually expect to see a decrease in perf, at least for this PR, as these calls no longer have access to the cached AssetRecords.

We can make that perf back once we make this async (or consider threading through a loading context), just want to avoid merging this in a vacuum if it makes a big difference (it might not).

python_modules/dagster/dagster/_core/storage/partition_status_cache.py Outdated Show resolved Hide resolved
python_modules/dagster/dagster/_core/asset_graph_view/asset_graph_view.py Outdated Show resolved Hide resolved
@briantu briantu force-pushed the briantu/make-asset-status-cache-value-loadable branch 2 times, most recently from 4d3c2cc to 2c11e72 Compare October 14, 2024 19:18
@briantu briantu changed the base branch from master to briantu/make-da-evaluation-optionally-async October 14, 2024 19:20
@briantu briantu assigned briantu and unassigned briantu Oct 14, 2024
This was referenced Oct 14, 2024
Merged
Merged
Merged
Closed
@briantu briantu force-pushed the briantu/make-da-evaluation-optionally-async branch from 720ded4 to 34a2997 Compare October 15, 2024 22:23
@briantu briantu force-pushed the briantu/make-asset-status-cache-value-loadable branch from 2c11e72 to 99479fd Compare October 15, 2024 22:24
@briantu briantu force-pushed the briantu/make-da-evaluation-optionally-async branch 2 times, most recently from 73818b7 to d56c073 Compare October 15, 2024 22:42
@briantu briantu force-pushed the briantu/make-asset-status-cache-value-loadable branch from 99479fd to 2257294 Compare October 15, 2024 22:42
@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/make-asset-status-cache-value-loadable branch from 2257294 to ea6002b Compare October 17, 2024 19:01
@briantu briantu force-pushed the briantu/make-da-evaluation-optionally-async branch from def9fe2 to 4a164ec Compare October 17, 2024 20:36
@briantu briantu force-pushed the briantu/make-asset-status-cache-value-loadable branch from ea6002b to 41e4571 Compare October 17, 2024 20:36
@briantu briantu force-pushed the briantu/make-da-evaluation-optionally-async branch from 4a164ec to 7fc9c05 Compare October 18, 2024 17:59
@briantu briantu force-pushed the briantu/make-asset-status-cache-value-loadable branch from 41e4571 to e5dcce0 Compare October 18, 2024 17:59
@briantu briantu mentioned this pull request Oct 18, 2024
Merged
@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/make-asset-status-cache-value-loadable branch from e5dcce0 to f57fd3a Compare October 18, 2024 22:49
@briantu briantu mentioned this pull request Oct 18, 2024
Merged
@briantu briantu force-pushed the briantu/make-da-evaluation-optionally-async branch from 0f48def to 2757128 Compare October 18, 2024 23:03
@briantu briantu force-pushed the briantu/make-asset-status-cache-value-loadable branch from f57fd3a to f3263a7 Compare October 18, 2024 23:03
@briantu briantu force-pushed the briantu/make-da-evaluation-optionally-async branch from 2757128 to 41914e8 Compare October 21, 2024 16:41
@briantu briantu force-pushed the briantu/make-asset-status-cache-value-loadable branch from f3263a7 to 31ab1fd Compare October 21, 2024 16:41
@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/make-asset-status-cache-value-loadable branch from 31ab1fd to 5790650 Compare October 21, 2024 19:52
@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/make-asset-status-cache-value-loadable branch from 5ae90f1 to 62c38f6 Compare October 21, 2024 22:00
@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/make-asset-status-cache-value-loadable branch from 62c38f6 to 2648efa Compare October 22, 2024 00:45
@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/make-asset-status-cache-value-loadable branch from 2648efa to d696688 Compare October 22, 2024 19:39
@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/make-asset-status-cache-value-loadable branch from d696688 to 8360c8c Compare October 22, 2024 20:24
@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/make-asset-status-cache-value-loadable branch from 8360c8c to 98cf324 Compare October 22, 2024 20:31
@briantu briantu force-pushed the briantu/make-da-evaluation-optionally-async branch from b115f84 to 7e354ee Compare October 22, 2024 21:13
@briantu briantu force-pushed the briantu/make-asset-status-cache-value-loadable branch from 98cf324 to 7b08a91 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:22 PM EDT: Graphite rebased this pull request as part of a merge.
  • Oct 22, 6:23 PM EDT: A user merged this pull request with Graphite.

@briantu briantu changed the base branch from briantu/make-da-evaluation-optionally-async to graphite-base/25200 October 22, 2024 22:17
@briantu briantu changed the base branch from graphite-base/25200 to master October 22, 2024 22:19
@briantu briantu force-pushed the briantu/make-asset-status-cache-value-loadable branch from 7b08a91 to 253a599 Compare October 22, 2024 22:21
@briantu briantu merged commit 65c5af3 into master Oct 22, 2024
1 check failed
@briantu briantu deleted the briantu/make-asset-status-cache-value-loadable branch October 22, 2024 22:23
Grzyblon pushed a commit to Grzyblon/dagster that referenced this pull request Oct 26, 2024
@briantu @Grzyblon
Make AssetStatusCacheValue inherit InstanceLoadableBy (dagster-io#25200)
365309e 
## Summary & Motivation
We want to make `AssetStatusCacheValue` follow the dataloader pattern as this will help optimize DA evaluation. This PR just uses `AssetStatusCacheValue.blocking_get()`, but will switch to using `AssetStatusCacheValue.gen()` when making things async in a follow up PR.

## 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