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

Make input partitions methods based on asset key not input name #19027

Conversation

jamiedemaria
Copy link
Contributor

@jamiedemaria jamiedemaria commented Jan 4, 2024

Summary & Motivation

One of the changes I'd like to make to the partition methods on the context is to allow them to accept CoercibleToAssetKey rather than input name. This will mean users wont need to map back and forth between asset key and input name and will remove the I/O manager-centric term "input".

To achieve this, the top level partition functions in the StepExecutionContext need to accept asset keys rather then input name. Right now the pattern is like this

class OpExecutionContext:

   def asset_partition_key_for_input(input_name):
        self.step_execution_context.asset_partition_key_for_input(input_name)

class StepExecutionContext:
    def asset_partition_key_for_input(input_name):
         asset_key = self.get_asset_key_for_input(input_name)
         ...

And I'd like it to be like this:

class OpExecutionContext:

   def asset_partition_key_for_input(input_name):
       asset_key = self.step_execution_context.get_asset_key_for_input(input_name)
        self.step_execution_context.asset_partition_key_for_asset(asset_key)

class StepExecutionContext:
    def asset_partition_key_for_asset(asset_key):
         ...

So that we can support this in AssetExecutionContext

class AssetExecutionContext:

   def partition_key_for_upstream_asset(asset_key: CoercibleToAssetKey):
        self.step_execution_context.asset_partition_key_for_asset(AssetKey.from_coercible(asset_key))

How I Tested These Changes

@jamiedemaria
Copy link
Contributor Author

jamiedemaria commented Jan 4, 2024

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@jamiedemaria jamiedemaria force-pushed the jamie/upstream-materialization-event branch from 310782a to 3e05391 Compare January 10, 2024 17:13
@jamiedemaria jamiedemaria force-pushed the jamie/partition-methods-asset-key-based branch from 97d5e6f to f062952 Compare January 10, 2024 17:13
@jamiedemaria jamiedemaria force-pushed the jamie/upstream-materialization-event branch from 3e05391 to 2149c6b Compare January 12, 2024 19:48
@jamiedemaria jamiedemaria force-pushed the jamie/partition-methods-asset-key-based branch from f062952 to 5331a95 Compare January 12, 2024 19:48
@jamiedemaria jamiedemaria force-pushed the jamie/upstream-materialization-event branch from 2149c6b to e68b4af Compare January 16, 2024 17:37
@jamiedemaria jamiedemaria force-pushed the jamie/partition-methods-asset-key-based branch from 5331a95 to c47a59d Compare January 16, 2024 17:37
@jamiedemaria jamiedemaria force-pushed the jamie/partition-methods-asset-key-based branch from c47a59d to 0cd7791 Compare January 16, 2024 18:15
@jamiedemaria jamiedemaria force-pushed the jamie/upstream-materialization-event branch from 24176a1 to e1a4076 Compare January 17, 2024 21:13
@jamiedemaria jamiedemaria force-pushed the jamie/partition-methods-asset-key-based branch from 0cd7791 to fe5fdbf Compare January 17, 2024 21:13
@jamiedemaria jamiedemaria marked this pull request as ready for review January 18, 2024 20:15
@@ -1109,22 +1112,16 @@ def get_output_asset_keys(self) -> AbstractSet[AssetKey]:
output_keys.add(asset_info.key)
return output_keys

def has_asset_partitions_for_input(self, input_name: str) -> bool:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's the deprecation policy for these methods on StepExecutionContext? Are they considered public? If so i'll retain versions of these methods that just call their asset_key based counterparts after converting the input name to an asset_key

Copy link
Contributor

Choose a reason for hiding this comment

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

Methods are only public if:

  • They're on a publicly-exported class
  • They have the @public decorator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok - these are public then. I'll add the method names back

Copy link
Contributor

Choose a reason for hiding this comment

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

This one at least doesn't have the @public decorator, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, but StepExecutionContext is publicly exported class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wait... was the list of conditions an AND not an OR?

Copy link
Contributor

@sryza sryza Jan 19, 2024

Choose a reason for hiding this comment

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

ah oops yes, an AND

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok i can revert that commit lol

@jamiedemaria jamiedemaria force-pushed the jamie/upstream-materialization-event branch from 401ba83 to 54cf22d Compare January 30, 2024 20:19
@jamiedemaria jamiedemaria force-pushed the jamie/partition-methods-asset-key-based branch from 2f56390 to d2ab183 Compare January 30, 2024 20:19
@jamiedemaria jamiedemaria force-pushed the jamie/upstream-materialization-event branch 2 times, most recently from ac6877e to d572c35 Compare February 2, 2024 19:51
@jamiedemaria
Copy link
Contributor Author

converting back to draft to get this out of review queues until context work is re-prioritized

@jamiedemaria jamiedemaria marked this pull request as draft February 6, 2024 18:00
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.

2 participants