[runless] report_asset_materialization endpoint #16602

alangenfeld · 2023-09-18T19:50:11Z

add a rest endpoint one can post to to report runless asset materializations

the endpoint /report_asset_materialization/ takes

asset key
- can be passed as url path, where / is the multipart delimiter like in from_user_string
- as query param asset_key, which expects the db encoding of json array of strings / single string
- as json post body with value passed directly to AssetKey
other properties
- passed as query params or json encoded post body

How I Tested These Changes

added test coverage

alangenfeld · 2023-09-18T19:50:22Z

Current dependencies on/for this PR:

master
- PR [runless] report_asset_materialization endpoint #16602 👈

This comment was auto-generated by Graphite.

alangenfeld · 2023-09-18T19:51:05Z

python_modules/dagster-webserver/dagster_webserver/webserver.py

@@ -323,6 +405,11 @@ def build_routes(self):
                    "/download_debug/{run_id:str}",
                    self.download_debug_file_endpoint,
                ),
+                Route(
+                    "/report_asset_materialization/{asset_key:path}",


differs from report_runless_asset_event naming since we are encoding the type as the part of the url path to allow for ease of use

schrockn

what about multi-part asset keys?

multipart key support added

alangenfeld · 2023-09-18T21:42:46Z

scripts/run-pyright.py

+            capture_output=True,
+            shell=True,
+            text=True,
+            check=False,  # dont raise on non zero exit codes


before https://buildkite.com/dagster/dagster/builds/66428#018aa9e4-08bc-4e45-9a68-33b462caf790

after https://buildkite.com/dagster/dagster/builds/66443#018aaa22-fb07-4dbd-8eb8-1670258e4737

schrockn

Still looking for test case that has multipart keys

alangenfeld · 2023-09-20T01:09:07Z

python_modules/dagster-webserver/dagster_webserver_tests/webserver/test_app.py

+    # multipart key
+    long_key = AssetKey(["foo", "bar", "baz"])
+    response = test_client.post(
+        f"/report_asset_materialization/{long_key.to_user_string()}",
+    )
+    assert response.status_code == 200
+    evt = instance.get_latest_materialization_event(long_key)
+    assert evt


multipart key test here

can you add assertion against fully expanded url (e.g. assert url == "report_asset_materialization/foo/bar/baz")

can you add test when asset key components contain "/"

I'm concerned about the answers to 2.

opted to support accepting asset_key via query params or post body to provide ways to workaround / in the asset key content. Could not find a clean path trying to deal url encoded / values being part of the path since the web framework resolves those encodings

see inline

github-actions · 2023-09-20T18:55:49Z

Deploy preview for dagit-storybook ready!

✅ Preview
https://dagit-storybook-5huvwz0tk-elementl.vercel.app
https://al-09-18--runless-report-asset-materialization-endpoint.components-storybook.dagster-docs.io

Built with commit eb5f3b3.
This pull request is being automatically deployed with vercel-action

github-actions · 2023-09-20T18:56:10Z

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-b9q35vn7e-elementl.vercel.app
https://al-09-18--runless-report-asset-materialization-endpoint.dagster.dagster-docs.io

Direct link to changed pages:

https://dagster-docs-b9q35vn7e-elementl.vercel.app
https://al-09-18--runless-report-asset-materialization-endpoint.dagster.dagster-docs.io/concepts/code-locations

github-actions · 2023-09-20T18:59:36Z

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-bieiapwrb-elementl.vercel.app
https://al-09-18--runless-report-asset-materialization-endpoint.core-storybook.dagster-docs.io

Built with commit b169682.
This pull request is being automatically deployed with vercel-action

schrockn

Cool.

Two questions:

Are we sure we want an endpoint per type instead of a single endpoint for all event types?
Can we put something in place to make sure the REST endpoint stays up-to-date with ExtContext methods?

Once possible path to doing this is writing an ExtContext implementation that hits the REST endpoint.

schrockn · 2023-09-21T16:35:00Z

python_modules/dagster-webserver/dagster_webserver/webserver.py

+            )
+
+        asset_key = None
+        if request.path_params.get("asset_key"):


Can we make sure we write a test or something that ensures that this endpoint stays up-to-date with the upcomingreport_asset_materialization method on ExtContext?

alangenfeld · 2023-09-21T20:16:04Z

Are we sure we want an endpoint per type instead of a single endpoint for all event types?

Not sure. What pushed me to the current scheme was trying to support the simple base case of only posting to a path which led me to encoding the event type in the path. Some other ideas:

we could still have one "endpoint" and still encode it in the path /report_asset/<event_type>/<asset>/<key>/<parts>
we could have one endpoint and assume materialization by default, take some type arg
Definitely open to other schemes as well

github-actions · 2023-09-21T21:04:25Z

Deploy preview for dagster-university ready!

✅ Preview
https://dagster-university-ifjestnn3-elementl.vercel.app
https://al-09-18--runless-report-asset-materialization-endpoint.dagster-university.dagster-docs.io

Built with commit b169682.
This pull request is being automatically deployed with vercel-action

alangenfeld · 2023-09-21T21:45:44Z

python_modules/dagster-webserver/dagster_webserver_tests/webserver/test_app.py

+    # things that are missing on ExtContext.report_asset_materialization
+    KNOWN_DIFF = {"partition", "description"}


should these get added to ExtContext.report_asset_materialization or is the premise of keeping these things in alignment not right?

alangenfeld · 2023-09-21T21:46:08Z

python_modules/dagster-webserver/dagster_webserver/webserver.py

+        tags = None
+        if ReportAssetMatParam.data_version in json_body:
+            tags = {
+                DATA_VERSION_TAG: json_body[ReportAssetMatParam.data_version],
+                DATA_VERSION_IS_USER_PROVIDED_TAG: "true",
+            }
+        elif ReportAssetMatParam.data_version in request.query_params:
+            tags = {
+                DATA_VERSION_TAG: request.query_params[ReportAssetMatParam.data_version],
+                DATA_VERSION_IS_USER_PROVIDED_TAG: "true",
+            }


@smackesey to confirm this data_version handling

for conversing on report API sync

alangenfeld requested a review from schrockn September 18, 2023 19:50

alangenfeld commented Sep 18, 2023

View reviewed changes

schrockn previously requested changes Sep 18, 2023

View reviewed changes

alangenfeld force-pushed the al/09-18-_runless_report_asset_materialization_endpoint branch from 46c8ce8 to 85aace8 Compare September 18, 2023 20:03

alangenfeld force-pushed the al/09-18-_runless_report_asset_materialization_endpoint branch 2 times, most recently from 36ec391 to 7b6651d Compare September 18, 2023 21:41

alangenfeld commented Sep 18, 2023

View reviewed changes

alangenfeld force-pushed the al/09-18-_runless_report_asset_materialization_endpoint branch from 7b6651d to 807f55b Compare September 19, 2023 17:33

schrockn previously requested changes Sep 20, 2023

View reviewed changes

alangenfeld commented Sep 20, 2023

View reviewed changes

alangenfeld force-pushed the al/09-18-_runless_report_asset_materialization_endpoint branch 2 times, most recently from eb5f3b3 to b387d7b Compare September 20, 2023 18:54

alangenfeld force-pushed the al/09-18-_runless_report_asset_materialization_endpoint branch from b387d7b to 5618d6d Compare September 20, 2023 18:59

alangenfeld force-pushed the al/09-18-_runless_report_asset_materialization_endpoint branch 3 times, most recently from ce88e4a to 5473048 Compare September 20, 2023 21:13

schrockn previously requested changes Sep 21, 2023

View reviewed changes

alangenfeld requested a review from smackesey September 21, 2023 21:01

alangenfeld force-pushed the al/09-18-_runless_report_asset_materialization_endpoint branch from 5473048 to b169682 Compare September 21, 2023 21:01

alangenfeld force-pushed the al/09-18-_runless_report_asset_materialization_endpoint branch 3 times, most recently from f835959 to 27696c0 Compare September 21, 2023 21:37

alangenfeld force-pushed the al/09-18-_runless_report_asset_materialization_endpoint branch from 27696c0 to 7ee1775 Compare September 21, 2023 21:44

alangenfeld commented Sep 21, 2023

View reviewed changes

alangenfeld requested a review from Ramshackle-Jamathon October 3, 2023 14:09

[runless] report_asset_materialization endpoint

22219ad

Ramshackle-Jamathon force-pushed the al/09-18-_runless_report_asset_materialization_endpoint branch from 7ee1775 to 22219ad Compare October 3, 2023 18:31

Ramshackle-Jamathon approved these changes Oct 6, 2023

View reviewed changes

fixups

fd9c509

alangenfeld merged commit cedfc82 into master Oct 6, 2023

alangenfeld deleted the al/09-18-_runless_report_asset_materialization_endpoint branch October 6, 2023 21:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[runless] report_asset_materialization endpoint #16602

[runless] report_asset_materialization endpoint #16602

alangenfeld commented Sep 18, 2023 •

edited

Loading

alangenfeld commented Sep 18, 2023

alangenfeld Sep 18, 2023

schrockn left a comment

alangenfeld Sep 18, 2023

schrockn left a comment

alangenfeld Sep 20, 2023

schrockn Sep 20, 2023

alangenfeld Sep 20, 2023

github-actions bot commented Sep 20, 2023

github-actions bot commented Sep 20, 2023

github-actions bot commented Sep 20, 2023 •

edited

Loading

schrockn left a comment

schrockn Sep 21, 2023

alangenfeld commented Sep 21, 2023

github-actions bot commented Sep 21, 2023

alangenfeld Sep 21, 2023

alangenfeld Sep 21, 2023

		# things that are missing on ExtContext.report_asset_materialization
		KNOWN_DIFF = {"partition", "description"}

[runless] report_asset_materialization endpoint #16602

[runless] report_asset_materialization endpoint #16602

Conversation

alangenfeld commented Sep 18, 2023 • edited Loading

How I Tested These Changes

alangenfeld commented Sep 18, 2023

Choose a reason for hiding this comment

schrockn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schrockn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 20, 2023

github-actions bot commented Sep 20, 2023

github-actions bot commented Sep 20, 2023 • edited Loading

schrockn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alangenfeld commented Sep 21, 2023

github-actions bot commented Sep 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alangenfeld commented Sep 18, 2023 •

edited

Loading

github-actions bot commented Sep 20, 2023 •

edited

Loading