[components] Templating for asset_attributes #26633
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 stack of pull requests is managed by Graphite. Learn more about stacking. |
OwenKephart
force-pushed
the
12-20-_components_templating_for_asset_attributes
branch
from
813dc40 to
483060a
Compare
OwenKephart
force-pushed
the
12-20-_components_templating_for_asset_attributes
branch
from
483060a to
38baab7
Compare
schrockn
reviewed
Dec 20, 2024
| self, | ||
| val: Any, | ||
| valpath: Optional[Sequence[Union[str, int]]], | ||
| should_render: Callable[[Sequence[Union[str, int]]], bool], |
There was a problem hiding this comment.
I'm not following this should_render stuff. Under what conditions will this be true versus false?
There was a problem hiding this comment.
I'm still finding this code path quite confusing given what is happening here should be pretty straightforward?
There was a problem hiding this comment.
Updated the names in the PR and added some comments, but basically there's just two options for the should_render function:
- a function that always returns true (this is what translators and the like use -- they will greedily render any values they come across)
- a function that inspects the json schema of the target params type to determine if we're deferring or not (this is what we use in the pre-processing stage to keep some fields unrendered)
schrockn
reviewed
Dec 20, 2024
python_modules/libraries/dagster-components/dagster_components/core/component_rendering.py
Outdated
Show resolved
Hide resolved
OwenKephart
force-pushed
the
12-20-_components_templating_for_asset_attributes
branch
from
38baab7 to
67dc5d0
Compare
schrockn
previously requested changes
Dec 20, 2024
OwenKephart
force-pushed
the
12-20-_components_templating_for_asset_attributes
branch
from
67dc5d0 to
3deb344
Compare
OwenKephart
force-pushed
the
12-20-_components_templating_for_asset_attributes
branch
from
3deb344 to
7eb1d13
Compare
schrockn
reviewed
Dec 23, 2024
python_modules/libraries/dagster-components/dagster_components/core/component_rendering.py
Outdated
Show resolved
Hide resolved
OwenKephart
force-pushed
the
12-20-_components_templating_for_asset_attributes
branch
from
7eb1d13 to
8fb785a
Compare
schrockn
requested changes
Dec 30, 2024
OwenKephart
force-pushed
the
12-20-_components_templating_for_asset_attributes
branch
from
8fb785a to
f4d3125
Compare
schrockn
approved these changes
Dec 31, 2024
OwenKephart
force-pushed
the
12-20-_components_templating_for_asset_attributes
branch
from
f4d3125 to
c4b98aa
Compare
OwenKephart
deleted the
12-20-_components_templating_for_asset_attributes
branch
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 adds a rendering scope to AssetAttributes, allowing its values to depend on the spec that's currently being evaluated. This is useful for setting properties of a spec based off of other properties (e.g. based off of a tag, set an automation condition)
Refactors component_rendering.py to be more class-based
How I Tested These Changes
Changelog
NOCHANGELOG