Pass loading context through _blocking_batch_load #25377
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Oct 18, 2024
This stack of pull requests is managed by Graphite. Learn more about stacking. |
briantu
force-pushed
the
briantu/make-compute-subset-async
branch
from
206d74c to
9291fd1
Compare
briantu
force-pushed
the
briantu/pass-loading-context-in-batch
branch
from
47d27bd to
18a7084
Compare
briantu
force-pushed
the
briantu/make-compute-subset-async
branch
from
9291fd1 to
9be1378
Compare
briantu
force-pushed
the
briantu/pass-loading-context-in-batch
branch
from
18a7084 to
c6bd3cd
Compare
briantu
force-pushed
the
briantu/make-compute-subset-async
branch
from
9be1378 to
4a1a5a3
Compare
briantu
force-pushed
the
briantu/pass-loading-context-in-batch
branch
from
c6bd3cd to
2e65c08
Compare
alangenfeld
reviewed
Oct 21, 2024
| if not issubclass(ttype, InstanceLoadableBy): | ||
| check.failed(f"{ttype} is not Loadable") | ||
|
|
||
| batch_load_fn = partial(ttype._batch_load, instance=self.instance) # noqa | ||
| blocking_batch_load_fn = partial(ttype._blocking_batch_load, instance=self.instance) # noqa |
There was a problem hiding this comment.
when I first set this up I imagined we might have other _X_LoadableBy which would bind _X_ instead of the instance for fetching, so in this case ContextLoadableBy
i'm not sure that having different flavors is necessary in the end here - but we might want to drop the Instance part of the naming for LoadableBy interface if we just want to always pass the context
There was a problem hiding this comment.
Yeah my thinking was that in a world where there are multiple things that we want to load with (i.e. multiple Xs), there would likely conceivably be singular objects that would want more than one of those Xs, and so at that point we'd probably just do this change anyway.
Agree LoadableBy would be a better name (maybe that could be a followup to this PR?)
There was a problem hiding this comment.
follow up or in this PR is fine with me
briantu
force-pushed
the
briantu/make-compute-subset-async
branch
from
4a1a5a3 to
0535e53
Compare
briantu
force-pushed
the
briantu/pass-loading-context-in-batch
branch
from
2e65c08 to
e66b775
Compare
briantu
force-pushed
the
briantu/make-compute-subset-async
branch
from
0535e53 to
647eb5d
Compare
briantu
force-pushed
the
briantu/pass-loading-context-in-batch
branch
from
e66b775 to
f4fdc53
Compare
briantu
force-pushed
the
briantu/make-compute-subset-async
branch
from
8721e6e to
6963946
Compare
briantu
force-pushed
the
briantu/pass-loading-context-in-batch
branch
from
161dd6c to
f9ba80d
Compare
alangenfeld
approved these changes
Oct 22, 2024
| if not issubclass(ttype, InstanceLoadableBy): | ||
| check.failed(f"{ttype} is not Loadable") | ||
|
|
||
| batch_load_fn = partial(ttype._batch_load, instance=self.instance) # noqa | ||
| blocking_batch_load_fn = partial(ttype._blocking_batch_load, instance=self.instance) # noqa |
There was a problem hiding this comment.
follow up or in this PR is fine with me
briantu
force-pushed
the
briantu/make-compute-subset-async
branch
from
6963946 to
34ba726
Compare
briantu
force-pushed
the
briantu/pass-loading-context-in-batch
branch
from
f9ba80d to
3643d67
Compare
briantu
force-pushed
the
briantu/make-compute-subset-async
branch
from
b0837ff to
6e4b0df
Compare
briantu
force-pushed
the
briantu/pass-loading-context-in-batch
branch
from
3643d67 to
51882eb
Compare
briantu
force-pushed
the
briantu/make-compute-subset-async
branch
from
6e4b0df to
59e884b
Compare
briantu
force-pushed
the
briantu/pass-loading-context-in-batch
branch
from
51882eb to
ef03572
Compare
briantu
force-pushed
the
briantu/make-compute-subset-async
branch
from
59e884b to
63c2dc8
Compare
briantu
force-pushed
the
briantu/pass-loading-context-in-batch
branch
from
b6f1c2f to
3df060b
Compare
briantu
changed the base branch from
briantu/make-compute-subset-async
to
graphite-base/25377
briantu
force-pushed
the
briantu/pass-loading-context-in-batch
branch
from
3df060b to
45d68da
Compare
Grzyblon
pushed a commit
to Grzyblon/dagster
that referenced
this pull request
Oct 26, 2024
## Summary & Motivation This lets us use the loading_context for get_asset_status_cache_values, which saves us from needing to re-fetch the asset records in here again. Copied from demo PR dagster-io#25270 ## How I Tested These Changes Perf test before/after ``` pytest python_modules/dagster/dagster_tests/definitions_tests/declarative_automation_tests/perf_tests/test_perf.py ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary & Motivation
This lets us use the loading_context for get_asset_status_cache_values, which saves us from needing to re-fetch the asset records in here again.
Copied from demo PR #25270
How I Tested These Changes
Perf test before/after