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

monitored_assets param for multi-asset sensor #11567

Merged
merged 1 commit into from Jan 18, 2023

Conversation

sryza
Copy link
Contributor

@sryza sryza commented Jan 6, 2023

Summary & Motivation

This takes advantage of the fact that multi-asset sensor is a relatively recent experimental API to do a breaking change. It replaces the asset_keys and asset_selection arguments to @multi_asset_sensor with a single monitored_assets argument.

The advantages:

  • We've received a number of user reports that, generally, asset selection parameters are finicky and difficult to get right. If we standardize everywhere on a single asset_selection parameter that accepts Union[AssetSelection, Sequence[AssetKey]], it would make life easier.
  • Avoids conflict with the asset_selection param on @sensor, which means something different.

Fixes #11558.

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 ↗︎

Name Status Preview Comments Updated
dagster ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Jan 18, 2023 at 9:56PM (UTC)
1 Ignored Deployment
Name Status Preview Comments Updated
dagit-storybook ⬜️ Ignored (Inspect) Jan 18, 2023 at 9:56PM (UTC)

@sryza
Copy link
Contributor Author

sryza commented Jan 6, 2023

Maybe we should just shorten it to monitored_selection

@NicolasPA
Copy link
Contributor

NicolasPA commented Jan 9, 2023

Or monitored_assets as opposed to run_assets in @sensor.

Copy link
Contributor

@jamiedemaria jamiedemaria left a comment

Choose a reason for hiding this comment

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

looks good!

@sryza sryza force-pushed the multi-asset-sensor-monitor-params branch from a758a23 to 067ce52 Compare January 18, 2023 21:54
@sryza sryza merged commit d446e7e into master Jan 18, 2023
@sryza sryza deleted the multi-asset-sensor-monitor-params branch January 18, 2023 23:12
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.

replace asset_keys and asset_selection params to multi_asset_sensor with monitored_asset_selection
3 participants