[ui] Dagre changes to support more flexible asset graph rendering #20525
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bengotow
commented
Mar 17, 2024
bengotow
commented
Mar 17, 2024
rexledesma
force-pushed
the
rl/column-lineage-metadata
branch
10 times, most recently
from
9c2119a to
c33f5da
Compare
salazarm
reviewed
Mar 18, 2024
Comment on lines
19
to
30
| export function useLastSavedZoomLevel( | ||
| viewportEl: MutableRefObject<SVGViewport | undefined>, | ||
| layout: import('../asset-graph/layout').AssetGraphLayout | null, | ||
| assetGraphId: string, | ||
| ) { | ||
| useEffect(() => { | ||
| if (viewportEl.current && layout) { | ||
| const lastZoomLevel = Number(getJSONForKey(LINEAGE_GRAPH_ZOOM_LEVEL)); | ||
| viewportEl.current.autocenter(false, lastZoomLevel); | ||
| viewportEl.current.focus(); | ||
| } | ||
| }, [viewportEl, layout, assetGraphId]); |
There was a problem hiding this comment.
You didn't actually use the assetGraphId here? I imagine the intent was to persist the zoom levels of different graphs independently?
There was a problem hiding this comment.
Ahh interesting... let me dig into this a bit, I think we may have been trying to make sure the zoom resets to the saved zoom when you select assets
There was a problem hiding this comment.
It looks like that was the case - i renamed this var so it's more obvious what it's doing here!
salazarm
reviewed
Mar 18, 2024
js_modules/dagster-ui/packages/ui-core/src/assets/lineage/useColumnLineageDataForAssets.tsx
Outdated
Show resolved
Hide resolved
salazarm
reviewed
Mar 18, 2024
js_modules/dagster-ui/packages/ui-core/src/assets/lineage/useColumnLineageDataForAssets.tsx
Show resolved
Hide resolved
rexledesma
force-pushed
the
rl/column-lineage-metadata
branch
from
c33f5da to
579cb49
Compare
added 9 commits
March 19, 2024 08:41
bengotow
force-pushed
the
bengotow-2024-03/column-lineage-metadata
branch
from
fc6a86e to
3fb3117
Compare
salazarm
approved these changes
Mar 19, 2024
added 6 commits
March 19, 2024 09:12
bengotow
pushed a commit
that referenced
this pull request
Mar 19, 2024
This reverts commit 4d77c2a.
bengotow
changed the title
salazarm
reviewed
Mar 20, 2024
| ); | ||
| }; | ||
|
|
||
| export function useColumnLineageDataForAssets(assetKeys: AssetKeyInput[]) { |
There was a problem hiding this comment.
Should this use LiveDataProvider for the batching/chunking polling benefits?
benpankow
pushed a commit
that referenced
this pull request
Mar 21, 2024
## Summary & Motivation This PR adds a new "Column" selector to the Asset > Lineage page for assets that emit `lineage` materialization event metadata. Choosing a column switches you to a column-oriented version of the graph, which supports the same upstream/downstream scoping + navigation as the normal lineage view, but shows column-level arrows and info. This PR makes a few changes to our asset layout system in order to support this new DAG styling: - The asset layout engine takes a wider range of it's hardcoded constants as configuration (previously just `direction`), and you can override Dagre layout settings. This allows us to run asset layout (with the worker, indexdb caching, etc), but with smaller `column` nodes. The options were already part of the cache keys, so this all works pretty smoothly! - I found an open PR on Dagre that fixes the issue with asset groups overlapping occasionally because the layout engine could not account for the presence of the "title bar" we render on them! I added the adjustments to our existing `dagre.patch` file. This is important in this PR because the nodes are the columns and the groups are the assets, so they overlap badly without this fix. ## How I Tested These Changes <img width="1728" alt="image" src="https://github.com/dagster-io/dagster/assets/1037212/0f5aebfd-cb5b-436e-b344-23b6a8f85870"> <img width="1563" alt="image" src="https://github.com/dagster-io/dagster/assets/1037212/4eccf376-4835-42b1-ad2c-590cb3917b1a"> <img width="1205" alt="image" src="https://github.com/dagster-io/dagster/assets/1037212/16f659a9-cb90-489b-b0ce-03a09a4e1a9c"> --------- Co-authored-by: bengotow <[email protected]>
PedramNavid
pushed a commit
that referenced
this pull request
Mar 28, 2024
## Summary & Motivation This PR adds a new "Column" selector to the Asset > Lineage page for assets that emit `lineage` materialization event metadata. Choosing a column switches you to a column-oriented version of the graph, which supports the same upstream/downstream scoping + navigation as the normal lineage view, but shows column-level arrows and info. This PR makes a few changes to our asset layout system in order to support this new DAG styling: - The asset layout engine takes a wider range of it's hardcoded constants as configuration (previously just `direction`), and you can override Dagre layout settings. This allows us to run asset layout (with the worker, indexdb caching, etc), but with smaller `column` nodes. The options were already part of the cache keys, so this all works pretty smoothly! - I found an open PR on Dagre that fixes the issue with asset groups overlapping occasionally because the layout engine could not account for the presence of the "title bar" we render on them! I added the adjustments to our existing `dagre.patch` file. This is important in this PR because the nodes are the columns and the groups are the assets, so they overlap badly without this fix. ## How I Tested These Changes <img width="1728" alt="image" src="https://github.com/dagster-io/dagster/assets/1037212/0f5aebfd-cb5b-436e-b344-23b6a8f85870"> <img width="1563" alt="image" src="https://github.com/dagster-io/dagster/assets/1037212/4eccf376-4835-42b1-ad2c-590cb3917b1a"> <img width="1205" alt="image" src="https://github.com/dagster-io/dagster/assets/1037212/16f659a9-cb90-489b-b0ce-03a09a4e1a9c"> --------- Co-authored-by: bengotow <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary & Motivation
The asset layout engine takes a wider range of it's hardcoded constants as configuration (previously just
direction), and you can override Dagre layout settings. This allows us to run asset layout (with the worker, indexdb caching, etc), but with smallercolumnnodes. The options were already part of the cache keys, so this all works pretty smoothly!I found an open PR on Dagre that fixes the issue with asset groups overlapping occasionally because the layout engine could not account for the presence of the "title bar" we render on them! I added the adjustments to our existing
dagre.patchfile. This is important in this PR because the nodes are the columns and the groups are the assets, so they overlap badly without this fix.