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

[external-assets] Standardize terminology on "execution set" #20140

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

smackesey
Copy link
Collaborator

@smackesey smackesey commented Feb 28, 2024

Summary & Motivation

The terminology around groups of assets/checks that must be executed together is currently a little mixed:

  • AssetGraph has get_required_multi_asset_keys and get_required_asset_and_check_keys
  • ExternalAssetNode and ExternalAssetCheck have a field atomic_execution_unit_id

It's not obvious that these are related to one another. This PR attempts to tighten up and standardize the terminology around the phrase "execution set":

  • AssetGraph.get_required_multi_asset_keys -> get_execution_set_asset_keys
  • AssetGraph.get_required_asset_and_check_keys -> get_execution_set_asset_and_check_keys
  • ExternalAsset{Node,Check}.atomic_execution_unit_id -> execution_set_id

It also tweaks the return value of the AssetGraph functions-- the current behavior is to return an empty set for subsettable assets, which does not make sense for "execution sets"-- this changes it to return a set with a single element (the passed asset key).

How I Tested These Changes

Existing test suite.

@smackesey smackesey force-pushed the sean/external-assets-execution-unit branch 3 times, most recently from b052dac to 59f3d5f Compare February 29, 2024 02:07
@smackesey smackesey force-pushed the sean/external-assets-asset-graph-tweaks branch from ac8a9c6 to 7228a63 Compare February 29, 2024 17:21
@smackesey smackesey force-pushed the sean/external-assets-execution-unit branch from 59f3d5f to 1fd1ee8 Compare February 29, 2024 17:21
@smackesey smackesey force-pushed the sean/external-assets-asset-graph-tweaks branch from 7228a63 to 2c4a5a8 Compare February 29, 2024 17:45
@smackesey smackesey force-pushed the sean/external-assets-execution-unit branch from 1fd1ee8 to b6ea604 Compare February 29, 2024 17:45
@smackesey smackesey force-pushed the sean/external-assets-asset-graph-tweaks branch from 2c4a5a8 to 0625561 Compare March 1, 2024 16:36
@smackesey smackesey changed the base branch from sean/external-assets-asset-graph-tweaks to sean/external-assets-repo-create-assets-for-undefined March 1, 2024 16:36
@smackesey smackesey force-pushed the sean/external-assets-execution-unit branch from b6ea604 to 37757be Compare March 1, 2024 16:36
@smackesey smackesey marked this pull request as ready for review March 1, 2024 18:05
@smackesey smackesey force-pushed the sean/external-assets-repo-create-assets-for-undefined branch from 101a9c6 to 599d04b Compare March 1, 2024 18:16
@smackesey smackesey force-pushed the sean/external-assets-execution-unit branch from 37757be to 9224109 Compare March 1, 2024 18:16
@smackesey smackesey force-pushed the sean/external-assets-repo-create-assets-for-undefined branch from 599d04b to 311d0ef Compare March 1, 2024 18:47
@smackesey smackesey force-pushed the sean/external-assets-execution-unit branch from 9224109 to 1d2272a Compare March 1, 2024 18:47
Copy link
Contributor

@OwenKephart OwenKephart left a comment

Choose a reason for hiding this comment

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

certainly an improvement 🙏

@smackesey smackesey force-pushed the sean/external-assets-repo-create-assets-for-undefined branch from 311d0ef to 3bb4896 Compare March 1, 2024 19:31
@smackesey smackesey force-pushed the sean/external-assets-execution-unit branch from 1d2272a to 5f73c57 Compare March 1, 2024 19:31
@smackesey smackesey force-pushed the sean/external-assets-repo-create-assets-for-undefined branch from 3bb4896 to d73e923 Compare March 2, 2024 20:43
@smackesey smackesey force-pushed the sean/external-assets-execution-unit branch from 5f73c57 to 6ad7465 Compare March 2, 2024 20:43
@smackesey
Copy link
Collaborator Author

So to be clear, an "execution unit" is defined as a "a set of assets and asset checks that must be executed together because they are attached to the same op and can_subset is false"?

Sounds accurate to me

@schrockn
Copy link
Member

schrockn commented Mar 5, 2024

Let's meet this AM to come up with some naming ideas for this concept. I don't think execution unit is particularly intuitive.

This is relevant conversation from Slack yesterday.

Screenshot 2024-03-05 at 7 49 30 AM

AssetSelection has a required_multi_asset_neighbors which isn't great either.

@smackesey smackesey force-pushed the sean/external-assets-repo-create-assets-for-undefined branch from 98c621b to edfd67f Compare March 5, 2024 14:26
@smackesey smackesey force-pushed the sean/external-assets-execution-unit branch from be91c37 to 6a92e61 Compare March 5, 2024 14:26
@smackesey smackesey force-pushed the sean/external-assets-repo-create-assets-for-undefined branch from edfd67f to d455262 Compare March 5, 2024 14:27
@smackesey smackesey force-pushed the sean/external-assets-execution-unit branch from 6a92e61 to c1ecf7a Compare March 5, 2024 14:27
@smackesey smackesey force-pushed the sean/external-assets-repo-create-assets-for-undefined branch from d455262 to 5a2551a Compare March 5, 2024 14:35
@smackesey smackesey force-pushed the sean/external-assets-execution-unit branch from c1ecf7a to 41726eb Compare March 5, 2024 14:35
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.

Per zoom convo:

execution unit -> execution set (the set of asset or check sets executed together)
atomic execution id -> execution set identifier

@schrockn
Copy link
Member

schrockn commented Mar 5, 2024

cc: @OwenKephart for your thoughts on this naming proposal

@smackesey smackesey force-pushed the sean/external-assets-repo-create-assets-for-undefined branch from 5a2551a to 6eadaed Compare March 5, 2024 18:12
@smackesey smackesey force-pushed the sean/external-assets-execution-unit branch from 41726eb to 018ca84 Compare March 5, 2024 18:12
Base automatically changed from sean/external-assets-repo-create-assets-for-undefined to master March 5, 2024 18:32
@smackesey smackesey force-pushed the sean/external-assets-execution-unit branch 3 times, most recently from f99d94b to 76f039d Compare March 5, 2024 19:28
@smackesey smackesey requested a review from schrockn March 5, 2024 19:29
@smackesey
Copy link
Collaborator Author

I've performed the rename to "execution_set_identifier"

@smackesey smackesey force-pushed the sean/external-assets-execution-unit branch from 76f039d to 8fae51a Compare March 5, 2024 19:43
@OwenKephart
Copy link
Contributor

@schrockn @smackesey to be honest, I'm not seeing a major difference between "execution unit" and "execution set" -- fine with this outcome.

@smackesey smackesey force-pushed the sean/external-assets-execution-unit branch from 8fae51a to 6cab24d Compare March 5, 2024 20:42
@schrockn
Copy link
Member

schrockn commented Mar 5, 2024

kn @smackesey to be honest, I'm not seeing a major difference between "execution unit" and "execution set" -- fine with this outcome.

@OwenKephart the thinking is that the word "set" indicates the "set of keys in an asset graph that must be executed together". This aligns with the can_subset language in our public API.

@smackesey
Copy link
Collaborator Author

@schrockn anything else you wanted to see in this PR

@smackesey smackesey changed the title [external-assets] Standardize terminology on "execution unit" [external-assets] Standardize terminology on "execution set" Mar 5, 2024
@smackesey smackesey merged commit ffb82f1 into master Mar 5, 2024
1 check passed
@smackesey smackesey deleted the sean/external-assets-execution-unit branch March 5, 2024 22:02
PedramNavid pushed a commit that referenced this pull request Mar 28, 2024
## Summary & Motivation

The terminology around groups of assets/checks that must be executed
together is currently a little mixed:

- `AssetGraph` has `get_required_multi_asset_keys` and
`get_required_asset_and_check_keys`
- `ExternalAssetNode` and `ExternalAssetCheck` have a field
`atomic_execution_unit_id`

It's not obvious that these are related to one another. This PR attempts
to tighten up and standardize the terminology around the phrase
"execution set":

- `AssetGraph.get_required_multi_asset_keys` ->
`get_execution_set_asset_keys`
- `AssetGraph.get_required_asset_and_check_keys` ->
`get_execution_set_asset_and_check_keys`
- `ExternalAsset{Node,Check}.atomic_execution_unit_id` ->
`execution_set_id`

It also tweaks the return value of the `AssetGraph` functions-- the
current behavior is to return an empty set for subsettable assets, which
does not make sense for "execution sets"-- this changes it to return a
set with a single element (the passed asset key).

## How I Tested These Changes

Existing test suite.
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.

3 participants