Handle async pipeline creation errors more gracefully#3158
Open
Conversation
Contributor
|
Hey @toji - is this PR ready for review? It's marked draft, but has 3 reviewers assigned. |
Collaborator
|
@ben-clayton I'm seeing no reviews requested, just 3 suggestions made by GitHub. |
Contributor
|
Right you are. Completely unable to parse the GitHub UI |
Member
Author
|
I think it's worthwhile to land this. Kai, could you PTAL? |
kainino0x
reviewed
Dec 15, 2023
| } else { | ||
| throw err; | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
Nothing fundamentally wrong with this change, but it is supposed to be totally fine to throw an exception from a test function (which awaiting a promise that rejects will do). I also don't want to cargo-cult this every place we use create*PipelineAsync if it's not necessary.
|
|
||
| // build the pipeline | ||
| return t.device.createComputePipeline({ | ||
| return t.device.createComputePipelineAsync({ |
Collaborator
There was a problem hiding this comment.
This is just an unrelated extra switch to async, right?
Collaborator
|
Any progress on this? |
Merged
8 tasks
Collaborator
8 tasks
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
These tests used to be producing validation errors which could be caught by the normal test harness, but after my changes in #3125 they're rejecting and throwing uncaught exceptions instead. This adds some try-catches around them and does an explicit
t.fail()if aGPUPipelineErroris thrown.Not landing just yet, because I haven't been able to repro original issue yet so this is just speculative. Should be easier to test early next week according to conversation in the linked issue.
Issue: #3157
Requirements for PR author:
.unimplemented()./** documented */and new helper files are found inhelper_index.txt.Requirements for reviewer sign-off:
When landing this PR, be sure to make any necessary issue status updates.