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.

Sign up for GitHub

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

Jump to bottom

RFC: @observable_source_asset adapter to generate AssetsDefinition and SensorDefinition #16623

Closed
wants to merge 3 commits into from
Closed

RFC: @observable_source_asset adapter to generate AssetsDefinition and SensorDefinition #16623

wants to merge 3 commits into from

Conversation

schrockn
Copy link
Member

@schrockn schrockn commented Sep 19, 2023 •
edited
Loading

Summary & Motivation

This PR demonstrates how we could write an adapter to achieve backwards compatibility with existing observable_source_asset declarations.

In a test case we have the following code:

    defs = Definitions(
        assets=[create_assets_def_from_source_asset(an_asset)],
        sensors=[sensor_def_from_observable_source_asset(an_asset)],
    )

If we so chose, we could push down calling those functions against the source asset to within the Definitions __init__ method machinery to make this completely automatic. Whether or not that is a good idea is a different discussion.

How I Tested These Changes

BK

This was referenced Sep 19, 2023
Merged
Merged
Closed
Merged
Merged
@schrockn
Copy link
Member Author

schrockn commented Sep 19, 2023 •
edited
Loading

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@schrockn schrockn changed the title observable assets in a user space sensor RFC: Enable sensors to emit arbitrary runless user events. Use this to implement a user-space observable source asset sensor Sep 19, 2023
schrockn
schrockn commented Sep 19, 2023
View reviewed changes
python_modules/dagster/dagster/_core/definitions/sensor_definition.py Outdated Show resolved Hide resolved
python_modules/dagster/dagster/_daemon/sensor.py Outdated
Comment on lines 682 to 684
for dagster_event in sensor_runtime_data.dagster_events or []:
instance.report_dagster_event(dagster_event, run_id="")

Copy link
Member Author

Choose a reason for hiding this comment

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

Similarly shouldn't hardcode "" run_id here

@schrockn schrockn force-pushed the source-asset-wrapping-5-explicit-sensor branch 2 times, most recently from 7d08a7d to 7350f94 Compare September 19, 2023 15:05
alangenfeld
alangenfeld reviewed Sep 19, 2023
View reviewed changes
Copy link
Member

@alangenfeld alangenfeld left a comment

Choose a reason for hiding this comment

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

I think I would add asset_event: Union["AssetMaterialization", "AssetObservation", "AssetCheckEvaluation"], to SensorResult and have it call instance.report_runless_asset_event() when it made it to back to the host process instead of this general dagster_event approach but I think the overall idea is sound.

@schrockn
Copy link
Member Author

I think I would add asset_event: Union["AssetMaterialization", "AssetObservation", "AssetCheckEvaluation"], to SensorResult and have it call instance.report_runless_asset_event() when it made it to back to the host process instead of this general dagster_event approach but I think the overall idea is sound.

Got it. It would be a List because we want to allow multiple events per tick

@schrockn schrockn force-pushed the source-asset-wrapping-4-new-obserable-asset-execution-type branch from c57f687 to 78ff1b8 Compare September 20, 2023 03:42
@schrockn schrockn force-pushed the source-asset-wrapping-5-explicit-sensor branch from 7350f94 to 895b2ca Compare September 20, 2023 03:42
@schrockn schrockn changed the title RFC: Enable sensors to emit arbitrary runless user events. Use this to implement a user-space observable source asset sensor Enable sensors to emit arbitrary runless user events. Use this to implement a user-space observable source asset sensor Sep 20, 2023
@schrockn schrockn marked this pull request as ready for review September 20, 2023 04:06
@schrockn
Copy link
Member Author

Incorporated your excellent suggestions @alangenfeld (so much cleaner) and this is ready to land.

alangenfeld
alangenfeld approved these changes Sep 20, 2023
View reviewed changes
Copy link
Member

@alangenfeld alangenfeld left a comment

Choose a reason for hiding this comment

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

should we mark this as experimental in some capacity?

python_modules/dagster/dagster/_daemon/sensor.py Outdated
@@ -679,6 +679,9 @@ def _evaluate_sensor(

assert isinstance(sensor_runtime_data, SensorExecutionData)

for asset_event in sensor_runtime_data.asset_events or []:
Copy link
Member

Choose a reason for hiding this comment

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

nit: do you need the or [] here given the coercion to empty list in new?

@schrockn
Copy link
Member Author

schrockn commented Sep 20, 2023 •
edited
Loading

Note @alangenfeld this is stacked on #16617 #16620 #16621

schrockn
schrockn commented Sep 20, 2023
View reviewed changes
Copy link
Member Author

@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.

I'm going to spin up a new PR with sensor changes only

schrockn
schrockn commented Sep 20, 2023
View reviewed changes
Copy link
Member Author

@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.

@alangenfeld #16658

@schrockn schrockn force-pushed the source-asset-wrapping-4-new-obserable-asset-execution-type branch 2 times, most recently from 02b6c10 to 9bb377c Compare September 22, 2023 02:56
@schrockn
observable assets in a user space sensor
dccfc42 
feedabck

Extract out observe_fn_to_op_compute_fn to it can be reused in other contexts

cp

feedback

experimental

cp
@schrockn schrockn force-pushed the source-asset-wrapping-5-explicit-sensor branch from 91cd31f to dccfc42 Compare September 22, 2023 02:58
@schrockn schrockn changed the title Enable sensors to emit arbitrary runless user events. Use this to implement a user-space observable source asset sensor Implement a user-space observable source asset sensor Sep 22, 2023
@schrockn schrockn changed the title Implement a user-space observable source asset sensor RFC: @observable_source_asset adapter to generate AssetsDefinition and SensorDefinition Sep 22, 2023
@schrockn schrockn requested a review from sryza September 22, 2023 03:41
@schrockn schrockn closed this Sep 22, 2023
@schrockn schrockn mentioned this pull request Sep 22, 2023
Closed
@schrockn
Copy link
Member Author

Decided to respin in new PR for clean discussion #16712

This was referenced Sep 22, 2023
Closed
Closed
@schrockn schrockn mentioned this pull request Oct 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alangenfeld alangenfeld Awaiting requested review from alangenfeld

@sryza sryza Awaiting requested review from sryza

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@schrockn @alangenfeld