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

Exploration of complete separation of metadata and compute: observable assets #16424

Closed
wants to merge 3 commits into from
Closed

Exploration of complete separation of metadata and compute: observable assets #16424

wants to merge 3 commits into from

Conversation

schrockn
Copy link
Member

Summary & Motivation

How I Tested These Changes

@schrockn
Copy link
Member Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@Ramshackle-Jamathon
Copy link
Contributor

Ramshackle-Jamathon commented Sep 11, 2023 •
edited
Loading

Nice, I'm still optimistic about this. Most of the questions I have about the ergonomics kinda boil down to the question of whether the Asset Graph is strictly derived from the python definitions or if its derived from some DB state + the python definitions.

  1. How are changes in the external assets reconciled with dagster?
  2. Do I need to create an asset definition in python for each ExternalAsset? Maybe require the python definition if you want to mark them as the upstream for dagster-native Assets?
  3. Many of the External systems might be dynamically generating new Assets, will those be automatically picked up? is there a way to detect and promote those into dagster Assets?

@petehunt
Copy link
Contributor

Seems really cool. A few thoughts:

  • Would be great if the UI had a copy-pastable curl command (that included a one-off auth token) to be able to update this metadata.
  • I think you should be able to "pull" metadata via a sensor in addition to the push method you have here.
  • Would these eventually replace source assets / observable source assets? Seems like it to me, and I would love for the number of nouns in the system to stay stable or trend downward over time.
  • Do you envision codegen tools to import these definitions from external systems eventually?

@schrockn schrockn changed the title Exploration of complete separation of metadata and compute Exploration of complete separation of metadata and compute: unmanaged assets Sep 11, 2023
@slopp
Copy link
Contributor

slopp commented Sep 11, 2023 

def report_asset_materialization(asset_key: str, metadata: dict):
    instance = DagsterInstance.get()
    dagster_event = DagsterEvent.from_external(
        event_type=DagsterEventType.ASSET_MATERIALIZATION,
        event_specific_data=StepMaterializationData(
            AssetMaterialization(asset_key=asset_key, metadata=metadata)
        )
    )
    instance.report_dagster_event(dagster_event, run_id="dummy")

This is almost exactly what some larger Dagster Cloud customers have done on their own. One such iuser created a job that looped through a bunch of asset partitions emitting materializations in order to represent an external backfill in Dagster. So a few questions:

  1. Will we support partitions and partition ranges?
  2. What is the performance implications? eg a big challenge this particular user faced was enumerating thousands of the events. Because each materialization event happens through a Python log handler, the performance became a major blocker. We'd want an option that could emit events for years worth of hourly partitions in, ideally, seconds or minutes (not hours as is the naive approach today).

@schrockn
Copy link
Member Author

Will we support partitions and partition ranges?

For sure

What is the performance implications?

I mean it does a db insert and a log per event. Is the problem for sure in the Python log handler? Either way it would be straightforward to provide a bulk insertion capability for backfills.

@schrockn
Copy link
Member Author

I'll write up a summary of this exploration later today. I didn't expect so many eyes on it.

@schrockn
Copy link
Member Author

Seems really cool. A few thoughts:

  • Would be great if the UI had a copy-pastable curl command (that included a one-off auth token) to be able to update this metadata.

Agreed. Great idea.

  • I think you should be able to "pull" metadata via a sensor in addition to the push method you have here.

That should not be a problem. I'll add an example.

  • Would these eventually replace source assets / observable source assets? Seems like it to me, and I would love for the number of nouns in the system to stay stable or trend downward over time.

Yes this should be straightforward. The only capability that this does not replicate is ad hoc observation from the UI. However I've wanted to consolidate the "observe sources" and "materialize" buttons for some time, so this would be a good excuse to address sure.

  • Do you envision codegen tools to import these definitions from external systems eventually?

For sure. AssetSpec is designed to make it easier to be a target for codegen as well as hydrate from other serializable formats (e.g. yaml).

@schrockn schrockn changed the title Exploration of complete separation of metadata and compute: unmanaged assets Exploration of complete separation of metadata and compute: observable assets Sep 12, 2023
schrockn
schrockn commented Sep 12, 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 moved this to an internal discussion (6739)

@alangenfeld alangenfeld mentioned this pull request Sep 14, 2023
Merged
@schrockn schrockn force-pushed the external-assets-exploration branch from f31ca5e to eb5daeb Compare September 16, 2023 09:09
@github-actions
Copy link

Deploy preview for dagster-university ready!

✅ Preview
https://dagster-university-649t1zugz-elementl.vercel.app
https://external-assets-exploration.dagster-university.dagster-docs.io

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

@github-actions
Copy link

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-eryv081oc-elementl.vercel.app
https://external-assets-exploration.core-storybook.dagster-docs.io

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

@github-actions
Copy link

Deploy preview for dagit-storybook ready!

✅ Preview
https://dagit-storybook-dz8z2p9c1-elementl.vercel.app
https://external-assets-exploration.components-storybook.dagster-docs.io

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

@github-actions
Copy link

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-g6v6yohe8-elementl.vercel.app
https://external-assets-exploration.dagster.dagster-docs.io

Direct link to changed pages:

alangenfeld added a commit that referenced this pull request Sep 18, 2023
@alangenfeld
[storage tests] runless events (#16520)
3b59f9b 
update storage tests to ensure runless asset events work across storages

related to #16424

## How I Tested These Changes
@schrockn schrockn closed this Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@schrockn @Ramshackle-Jamathon @petehunt @slopp