[gql] Expose partition mapping on AssetDependency #18119
Conversation
|
Current dependencies on/for this PR: This stack of pull requests is managed by Graphite. |
|
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit aeb1b72. |
clairelin135
force-pushed
the
11-16-claire/expose-partition-mapping-gql
branch
from
5ccccdd to
89346f7
Compare
| @@ -159,6 +166,11 @@ def resolve_asset(self, _graphene_info: ResolveInfo): | |||
| materialization_loader=self._latest_materialization_loader, | |||
| ) | |||
|
|
|||
| def resolve_partitionMapping(self, _graphene_info: ResolveInfo): | |||
There was a problem hiding this comment.
I know we haven't been super rigorous about this in the past, but I think it's worth adding a return type annotation here.
|
|
||
|
|
||
| class GraphenePartitionMapping(graphene.ObjectType): | ||
| name = graphene.NonNull(graphene.String) |
There was a problem hiding this comment.
Calling this class_name would be more clear.
| @@ -191,6 +197,9 @@ def get_downstream_partitions_for_partitions( | |||
| ) -> PartitionsSubset: | |||
| raise NotImplementedError() | |||
|
|
|||
| def __str__(self) -> str: | |||
There was a problem hiding this comment.
I think calling this description would be better, because it allows __str__ to show a more Dagster-developer-accessible version of the class's representation (with the class name and attributes).
python_modules/dagster/dagster/_core/definitions/partition_mapping.py
Outdated
Show resolved
Hide resolved
| @@ -292,6 +304,9 @@ def get_downstream_partitions_for_partitions( | |||
| ) | |||
| return downstream_partitions_def.empty_subset() | |||
|
|
|||
| def __str__(self) -> str: | |||
| return f"Each downstream partition depends on the following upstream partitions: {self.partition_keys}" | |||
There was a problem hiding this comment.
In some cases, this could be a large number of partitions. Should we be concerned?
There was a problem hiding this comment.
It is a little concerning, but for StaticPartitionsDefinitions our description string is all of the keys concatenated with a comma, which is probably worse
There was a problem hiding this comment.
We could just truncate and add an ellipsis on the front end if the string is too long?
There was a problem hiding this comment.
Got it. That sounds fine to me for now.
| @@ -728,6 +751,18 @@ def __new__( | |||
| ), | |||
| ) | |||
|
|
|||
| def __str__(self) -> str: | |||
| description = "\n ".join( | |||
There was a problem hiding this comment.
This one is complex enough that I think it's worth writing a test for it.
| type PartitionMapping { | ||
| name: String! | ||
| description: String! |
There was a problem hiding this comment.
This looks great to me! I'll have to think about how we show this on a per-dependency basis on the asset details page, but I think this is a great solution.
clairelin135
force-pushed
the
11-16-claire/expose-partition-mapping-gql
branch
from
89346f7 to
a02f8da
Compare
|
@sryza I updated this PR to address your feedback |
|
Deploy preview for dagster-university ready! ✅ Preview Built with commit a02f8da. |
Adds backend support to resolve #15720
This PR exposes the partition mapping on
GrapheneAssetDependency, which represents the "edge" in between an asset and its upstream. The partition mapping is only returned if it is defined on the asset (and not inferred). The reasoning here is to avoid introducing complexity in the UI when the user hasn't provided an explicit definition.It adds descriptions for partition mappings in case if we'd like to display that.