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

define_asset_job accept AssetKey and AssetsDefinition selections #11568

Merged
merged 1 commit into from Jan 9, 2023

Conversation

sryza
Copy link
Contributor

@sryza sryza commented Jan 6, 2023

Summary & Motivation

Enable passing a list of AssetKeys or AssetsDefinitions to the selection argument of define_asset_job.

How I Tested These Changes

bk

@vercel
Copy link

vercel bot commented Jan 6, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated
dagit-storybook ⬜️ Ignored (Inspect) Jan 9, 2023 at 5:51PM (UTC)
dagster ⬜️ Ignored (Inspect) Jan 9, 2023 at 5:51PM (UTC)

@NicolasPA
Copy link
Contributor

Thank you for this! It will feel much more natural to be able to directly pass a bunch of AssetsDefinition to an asset selection parameter.
I think this changed should be applied to every asset selection parameter, for example @asset_sensor and @multi_asset_sensor (related to #11567). Below before/after examples of the code simplification it leads to:

from dagster import asset, define_asset_job, AssetSelection, with_resources, multi_asset_sensor, RunRequest


@asset
def asset1():
    return [1, 2, 3]


@asset
def asset2(asset1):
    return asset1 + [4]


@asset
def asset3():
    return 1


# Instead of:
job1 = define_asset_job(name="job1", selection=AssetSelection.assets(asset1))
# Directly pass AssetsDefinition
job1 = define_asset_job(name="job1", selection=asset1)

# Instead of:
job2 = define_asset_job(name="job2", selection=AssetSelection.assets(asset1, asset2))
# pass List[AssetsDefinition]
job2 = define_asset_job(name="job2", selection=[asset1, asset2])

assets_with_resources = with_resources(
    [asset1, asset2],
    resource_defs={"foo": my_resource()}
)

# Instead of:
job3 = define_asset_job(name="job3", selection=AssetSelection.assets(*assets_with_resources))
# pass Sequence[AssetsDefinition]
job3 = define_asset_job(name="job3", selection=assets_with_resources)

# Instead of:
job4 = define_asset_job(name="job4", selection=AssetSelection.assets(asset3))
# pass AssetsDefinition
job4 = define_asset_job(name="job4", selection=asset3)


# Instead of:
@multi_asset_sensor(
    asset_selection=AssetSelection.assets(*assets_with_resources),
    job=job4,
)
def asset_1_and_2_sensor(context):
    return RunRequest()


# pass Sequence[AssetsDefinition]
@multi_asset_sensor(
    asset_selection=assets_with_resources,
    job=job4,
)
def asset_1_and_2_sensor(context):
    return RunRequest()

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.

Happy to approve as is, but thoughts on creating a CoercibleToAssetSelection type (similar to the CoercibleToAssetKey type)? This would make it easier to apply this same logic to other asset selection parameters.

@sryza sryza force-pushed the define-asset-job-selection branch from 5df02f3 to 7e559fa Compare January 9, 2023 17:51
@sryza
Copy link
Contributor Author

sryza commented Jan 9, 2023

Happy to approve as is, but thoughts on creating a CoercibleToAssetSelection type (similar to the CoercibleToAssetKey type)? This would make it easier to apply this same logic to other asset selection parameters.

I'm going to merge this in its present state, but yes, I think we should do this as soon as we add the same parameter to a second function/class.

@sryza sryza merged commit 89ae9b6 into master Jan 9, 2023
@sryza sryza deleted the define-asset-job-selection branch January 9, 2023 19:02
AlexanderVR pushed a commit to AlexanderVR/dagster that referenced this pull request Jan 10, 2023
…ster-io#11568)

### Summary & Motivation

Enable passing a list of AssetKeys or AssetsDefinitions to the
`selection` argument of `define_asset_job`.

### How I Tested These Changes

bk
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

3 participants