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.

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

[ui] Skip dimension matching check when dynamic partition added #19106

Merged

Conversation

clairelin135
Copy link
Contributor

@clairelin135 clairelin135 commented Jan 9, 2024

Fixes #17380.

Users were seeing the Attempting to show unified asset health for assets with dimension of different lengths error because the usePartitionHealthData query makes a separate request for partitions per asset, updating the state after each asset's request. When 2 assets are selected, the first request returns a response including the new partition, causing the error because the second asset's request has not returned yet.

Because the LaunchAssetChoosePartitionsDialog component already has an existing check to assert that each root partitions def in the asset selection is the same, we can skip the internal check. This PR adds a flag to bypass the "dimensions of different lengths" check.

Adds a test to preserve this behavior.

@clairelin135
Copy link
Contributor Author

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@clairelin135 clairelin135 force-pushed the 01-09-claire/add-flag-to-skip-dimension-matching-check branch from 7ef410f to 464c840 Compare January 9, 2024 18:40
Copy link

github-actions bot commented Jan 9, 2024

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-d0917p1ux-elementl.vercel.app
https://01-09-claire-add-flag-to-skip-dimension-matching-check.core-storybook.dagster-docs.io

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

@clairelin135 clairelin135 force-pushed the 01-09-claire/add-flag-to-skip-dimension-matching-check branch 2 times, most recently from edfc598 to 8d4047e Compare January 11, 2024 20:23
Copy link

github-actions bot commented Jan 11, 2024

Deploy preview for dagit-storybook ready!

✅ Preview
https://dagit-storybook-4jzfyrqgn-elementl.vercel.app
https://01-09-claire-add-flag-to-skip-dimension-matching-check.components-storybook.dagster-docs.io

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

Copy link

github-actions bot commented Jan 11, 2024

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-olhccskr8-elementl.vercel.app
https://01-09-claire-add-flag-to-skip-dimension-matching-check.dagster.dagster-docs.io

Direct link to changed pages:

@clairelin135 clairelin135 force-pushed the 01-09-claire/add-flag-to-skip-dimension-matching-check branch from 8d4047e to 852f133 Compare January 17, 2024 18:01
@@ -49,6 +49,9 @@ export class ErrorBoundary extends React.Component<ErrorBoundaryProps, ErrorBoun
}

componentDidCatch(error: Error, info: any) {
if (typeof jest !== 'undefined') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This ensures that in test cases an error is thrown instead of caught

@clairelin135 clairelin135 force-pushed the 01-09-claire/add-flag-to-skip-dimension-matching-check branch from 1f6bf8a to c448faf Compare January 17, 2024 18:23
@clairelin135 clairelin135 marked this pull request as ready for review January 17, 2024 19:16
Copy link
Contributor

@salazarm salazarm left a comment

Choose a reason for hiding this comment

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

🥂 This is great!

Copy link
Collaborator

@bengotow bengotow left a comment

Choose a reason for hiding this comment

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

Looks good to me! Hooray for new tests :-)

expect(assetASecondQueryMockResult).toHaveBeenCalled();
});

expect(errorMock).not.toHaveBeenCalled();
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove this expectation since we didn't use it, we relied on the error being thrown to fail the test as opposed to relying on an assertion failing to fail to the test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it, sounds good

@clairelin135 clairelin135 force-pushed the 01-09-claire/add-flag-to-skip-dimension-matching-check branch 2 times, most recently from 655676b to 8868cb6 Compare January 25, 2024 23:04
@clairelin135 clairelin135 force-pushed the 01-09-claire/add-flag-to-skip-dimension-matching-check branch from 8868cb6 to c1d0139 Compare January 25, 2024 23:18
@clairelin135 clairelin135 merged commit 4d0c69c into master Jan 25, 2024
2 checks passed
@clairelin135 clairelin135 deleted the 01-09-claire/add-flag-to-skip-dimension-matching-check branch January 25, 2024 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI error when adding a new partition to DynamicPartitionsDefinition inside a MultiPartitionsDefinition
3 participants