-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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.
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
[prototype] AssetExecutionContext partition methods directly on the context #19032
Conversation
) | ||
|
||
@public | ||
def upstream_partition_keys(self, key: CoercibleToAssetKey) -> Sequence[str]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make all of these accept a CoercibleToAssetDep
maybe? that way we can pass definitions around vs string matching on asset key? or maybe doing this is sufficiently good pattern
@asset(...)
def upstream():
...
@asset(deps=[upstream], ...)
def downstream(context):
context.upstream_partition_keys(upstream.key)
allowing CoercibleToAssetDep
could cause confusing scoping issues like this
@asset(...)
def upstream():
...
@asset(...)
def downstream(context, upstream):
context.upstream_partition_keys(upstream.key) # upstream doesn't have a key in this case since it's a loaded value
97d5e6f
to
f062952
Compare
9704201
to
2e86bcf
Compare
|
||
@public | ||
@property | ||
def has_partition_key(self) -> bool: # TODO - maybe this one can be replaced by something else? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from my understanding, this method is mostly used to check if a run is partitioned or not. In that case, I think a name like is_partitioned
, is_partitioned_materialization
/execution
/run
is better
f062952
to
5331a95
Compare
2e86bcf
to
592f799
Compare
@@ -33,7 +33,7 @@ def relativedelta(*args, **kwargs): | |||
metadata={"partition_expr": "LastModifiedDate"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file will not get merged. It's an example of what some of these apis will look like in use
b0e7745
to
215fed9
Compare
e55de50
to
5558635
Compare
215fed9
to
38b66f9
Compare
5558635
to
051390e
Compare
38b66f9
to
50c80ab
Compare
04fd627
to
0e1fc3e
Compare
6dde54c
to
f5f6756
Compare
0e1fc3e
to
dc6edf0
Compare
f5f6756
to
782de6a
Compare
dc6edf0
to
ccc4260
Compare
782de6a
to
2334f74
Compare
ccc4260
to
f416c05
Compare
2334f74
to
9d449ab
Compare
f416c05
to
51d46aa
Compare
9d449ab
to
902a82b
Compare
51d46aa
to
2670364
Compare
902a82b
to
7aa0437
Compare
2670364
to
2660075
Compare
7aa0437
to
baa4e2f
Compare
2660075
to
a5828e1
Compare
baa4e2f
to
587de6b
Compare
a5828e1
to
f5be566
Compare
587de6b
to
e6792e3
Compare
Summary & Motivation
This PR is a prototype for a potential API for partition methods on the
AssetExecutionContext
. It reduces the number of partition-related methods on the context by eliminatingoutput
methods (since the partition for an output will be the same for all outputs produced by the asset). The names of the methods are also updated to be asset-centric rather than input/output-centric and they will accept asset keys rather than input/output names.Hooli PR - dagster-io/hooli-data-eng-pipelines#59
DOP PR - https://github.com/dagster-io/internal/pull/7903
How I Tested These Changes