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.

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

[DS][35/n] Replace asset_subsets_with_metadata with asset_slices_with_metadata #21793

Conversation

OwenKephart
Copy link
Contributor

Summary & Motivation

As title -- we want to hide AssetSubset from the internals as much as possible.

How I Tested These Changes

Copy link
Contributor Author

OwenKephart commented May 10, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @OwenKephart and the rest of your teammates on Graphite Graphite

@OwenKephart OwenKephart changed the base branch from 05-07-Create_ChildCondition_condition to 05-07-Remove_some_AssetCondition_references May 10, 2024 22:44
@OwenKephart OwenKephart force-pushed the 05-10-Replace_asset_subsets_with_metadata_with_asset_slices_with_metadata branch from 4946a2f to eb565e8 Compare May 10, 2024 22:44
@OwenKephart OwenKephart requested a review from schrockn May 10, 2024 22:45
@OwenKephart OwenKephart changed the title [DS][x/n] Replace asset_subsets_with_metadata with asset_slices_with_metadata [DS][35/n] Replace asset_subsets_with_metadata with asset_slices_with_metadata May 10, 2024
@@ -263,9 +265,22 @@ def create(
context: "SchedulingContext",
true_slice: AssetSlice,
subsets_with_metadata: Sequence[AssetSubsetWithMetadata] = [],
slices_with_metadata: Sequence[Tuple[AssetSlice, MetadataMapping]] = [],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should introduce a new object here that capture an AssetSlice enrich with metadata, rather than relying on a tuple.

Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a lightweight abstraction to model this. I think it will be useful. I also want to get a general pattern going for associating arbitrary objects with an AssetSlice, because I believe we will want to do that quite a bit. I'm not saying build a super flexible, generalized abstract now, but a nice baby step is a lightweight object here.

@OwenKephart OwenKephart force-pushed the 05-07-Remove_some_AssetCondition_references branch from 8ac28d7 to 8e6c879 Compare May 13, 2024 17:30
@OwenKephart OwenKephart force-pushed the 05-10-Replace_asset_subsets_with_metadata_with_asset_slices_with_metadata branch from eb565e8 to b9a986c Compare May 13, 2024 17:30
@OwenKephart OwenKephart requested a review from schrockn May 13, 2024 17:35
@OwenKephart OwenKephart force-pushed the 05-07-Remove_some_AssetCondition_references branch from 8e6c879 to 06e29fe Compare May 13, 2024 17:51
@OwenKephart OwenKephart force-pushed the 05-10-Replace_asset_subsets_with_metadata_with_asset_slices_with_metadata branch from b9a986c to 97839e1 Compare May 13, 2024 17:54
from dagster._model import DagsterModel
from dagster._utils import utc_datetime_from_timestamp


class AssetSliceWithMetadata(NamedTuple):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok we can workshop this name later and generalize it but this is a good intermediate step.

@OwenKephart OwenKephart force-pushed the 05-07-Remove_some_AssetCondition_references branch from 06e29fe to a4c99d7 Compare May 14, 2024 13:47
@OwenKephart OwenKephart force-pushed the 05-10-Replace_asset_subsets_with_metadata_with_asset_slices_with_metadata branch from 97839e1 to fc4c2a0 Compare May 14, 2024 13:48
@OwenKephart OwenKephart force-pushed the 05-07-Remove_some_AssetCondition_references branch from a4c99d7 to d8ab103 Compare May 14, 2024 13:56
@OwenKephart OwenKephart force-pushed the 05-10-Replace_asset_subsets_with_metadata_with_asset_slices_with_metadata branch from fc4c2a0 to a5d3f72 Compare May 14, 2024 13:56
@OwenKephart OwenKephart force-pushed the 05-07-Remove_some_AssetCondition_references branch from d8ab103 to ded5c44 Compare May 14, 2024 13:59
@OwenKephart OwenKephart force-pushed the 05-10-Replace_asset_subsets_with_metadata_with_asset_slices_with_metadata branch from a5d3f72 to 407cd58 Compare May 14, 2024 13:59
@OwenKephart OwenKephart force-pushed the 05-07-Remove_some_AssetCondition_references branch from ded5c44 to 516f396 Compare May 14, 2024 14:14
@OwenKephart OwenKephart force-pushed the 05-10-Replace_asset_subsets_with_metadata_with_asset_slices_with_metadata branch from 407cd58 to b6d8453 Compare May 14, 2024 14:14
Copy link
Contributor Author

OwenKephart commented May 14, 2024

Merge activity

  • May 14, 10:38 AM EDT: @OwenKephart started a stack merge that includes this pull request via Graphite.
  • May 14, 11:03 AM EDT: Graphite rebased this pull request as part of a merge.
  • May 14, 11:04 AM EDT: @OwenKephart merged this pull request with Graphite.

@OwenKephart OwenKephart force-pushed the 05-07-Remove_some_AssetCondition_references branch 2 times, most recently from d91911f to 4fdd85a Compare May 14, 2024 15:00
Base automatically changed from 05-07-Remove_some_AssetCondition_references to master May 14, 2024 15:02
@OwenKephart OwenKephart force-pushed the 05-10-Replace_asset_subsets_with_metadata_with_asset_slices_with_metadata branch from b6d8453 to e374d83 Compare May 14, 2024 15:02
@OwenKephart OwenKephart merged commit 0474105 into master May 14, 2024
1 check was pending
@OwenKephart OwenKephart deleted the 05-10-Replace_asset_subsets_with_metadata_with_asset_slices_with_metadata branch May 14, 2024 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants