-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[ui] Show one asset run tag or “4 assets”, hover action for lineage (#…
…16449) ## Summary & Motivation This PR changes the asset tags that appear in the run table and on the run details page: 1) Rather than show 1-3 individual assets or a "6 assets" link in the run table or run details header, we now show either a single asset or "6 assets". This means the rendering is more compact in the 2-3 asset case since the tags are not rendered individually. 2) Instead of being directly clickable, hovering over the assets gives you the option to jump to a lineage view OR open the list of assets. For >1 asset, the downstream lineage option goes to the global asset graph with the "assetA*, assetB*" query. It's actually pretty slick. Right now in Dagit we don't have a "default behavior" for clicking tags that have tag actions -- you have to choose an action. I could see keeping the default "link" behavior we had before though... Before: ![image](https://github.com/dagster-io/dagster/assets/1037212/e09c7998-d92a-496f-96b4-bcdd7d48c253) ![image](https://github.com/dagster-io/dagster/assets/1037212/ea86bac2-bf6a-4c03-84b6-3c134c01db5f) ![image](https://github.com/dagster-io/dagster/assets/1037212/44c711e0-dfef-4213-acaf-deff4f70b489) After: ![image](https://github.com/dagster-io/dagster/assets/1037212/f81a4178-c3af-437a-bcba-1db760750244) ![image](https://github.com/dagster-io/dagster/assets/1037212/845dfd0b-b1b7-438b-8d70-b9130fba9fb1) ![image](https://github.com/dagster-io/dagster/assets/1037212/836cbb21-f485-4555-b82a-dfe6303cbeec) Also verified with multi-component asset keys: ![image](https://github.com/dagster-io/dagster/assets/1037212/882bced3-382c-4bdb-b420-93e32be2038e) ![image](https://github.com/dagster-io/dagster/assets/1037212/da6b77b9-6ff9-40ef-aced-235fb6677439) ## How I Tested These Changes I viewed the runs list and run details for a wide range of runs. --------- Co-authored-by: bengotow <[email protected]>
- Loading branch information
Showing
13 changed files
with
377 additions
and
278 deletions.
There are no files selected for viewing
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
36 changes: 36 additions & 0 deletions
36
...es/dagster-ui/packages/ui-core/src/assets/__tests__/globalAssetGraphPathToString.test.tsx
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
import { | ||
globalAssetGraphPathToString, | ||
globalAssetGraphPathForAssetsAndDescendants, | ||
} from '../globalAssetGraphPathToString'; | ||
|
||
// This file must be mocked because Jest can't handle `import.meta.url`. | ||
jest.mock('../../graph/asyncGraphLayout', () => ({})); | ||
|
||
describe('Global Graph URLs', () => { | ||
describe('globalAssetGraphPathToString', () => { | ||
it('should return a valid path given a selection and query', async () => { | ||
const url = globalAssetGraphPathToString({ | ||
opNames: ['foo_bar'], | ||
opsQuery: `foo*, bar, foo_bar++`, | ||
}); | ||
expect(url).toEqual(`/asset-groups~foo*%2C%20bar%2C%20foo_bar%2B%2B/foo_bar`); | ||
}); | ||
}); | ||
|
||
describe('globalAssetGraphPathForAssetsAndDescendants', () => { | ||
it('should return a valid path for a single asset', async () => { | ||
const url = globalAssetGraphPathForAssetsAndDescendants([{path: ['asset_0']}]); | ||
expect(url).toEqual(`/asset-groups~asset_0*/asset_0`); | ||
}); | ||
|
||
it('should avoid exceeding a 32k character URL', async () => { | ||
const keysLarge = new Array(900).fill(0).map((_, idx) => ({path: [`asset_${idx}`]})); | ||
const urlLarge = globalAssetGraphPathForAssetsAndDescendants(keysLarge); | ||
expect(urlLarge.length).toEqual(24986); | ||
|
||
const keysHuge = new Array(1500).fill(0).map((_, idx) => ({path: [`asset_${idx}`]})); | ||
const urlHuge = globalAssetGraphPathForAssetsAndDescendants(keysHuge); | ||
expect(urlHuge.length).toEqual(24399); // smaller because the selection is not passed | ||
}); | ||
}); | ||
}); |
30 changes: 30 additions & 0 deletions
30
js_modules/dagster-ui/packages/ui-core/src/assets/globalAssetGraphPathToString.tsx
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
import {tokenForAssetKey} from '../asset-graph/Utils'; | ||
import {AssetKeyInput} from '../graphql/types'; | ||
import { | ||
ExplorerPath, | ||
explorerPathFromString, | ||
explorerPathToString, | ||
} from '../pipelines/PipelinePathUtils'; | ||
|
||
// https://stackoverflow.com/questions/417142/what-is-the-maximum-length-of-a-url-in-different-browsers | ||
const URL_MAX_LENGTH = 32000; | ||
const __GLOBAL__ = '__GLOBAL__'; | ||
|
||
export function globalAssetGraphPathToString(path: Omit<ExplorerPath, 'pipelineName'>) { | ||
const str = explorerPathToString({...path, pipelineName: __GLOBAL__}).replace(__GLOBAL__, ''); | ||
return `/asset-groups${str}`; | ||
} | ||
|
||
export function globalAssetGraphPathFromString(pathName: string) { | ||
return explorerPathFromString(__GLOBAL__ + pathName || '/'); | ||
} | ||
|
||
export function globalAssetGraphPathForAssetsAndDescendants(assetKeys: AssetKeyInput[]) { | ||
// In a perfect world we populate ops query to "asset1*,asset2*" and then select the roots | ||
// by passing opNames. If we don't have enough characters to do both, just populate the ops | ||
// query. It might still be too long, but we tried. | ||
const opsQuery = assetKeys.map((a) => `${tokenForAssetKey(a)}*`).join(', '); | ||
const opNames = | ||
opsQuery.length > URL_MAX_LENGTH / 2 ? [] : [assetKeys.map(tokenForAssetKey).join(',')]; | ||
return globalAssetGraphPathToString({opNames, opsQuery}); | ||
} |
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
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
Oops, something went wrong.
7a0617c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deploy preview for dagit-core-storybook ready!
✅ Preview
https://dagit-core-storybook-o8c3wjxxg-elementl.vercel.app
Built with commit 7a0617c.
This pull request is being automatically deployed with vercel-action