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

Fix DeviceModifier #4103

Merged
merged 2 commits into from
Dec 18, 2024
Merged

Fix DeviceModifier #4103

merged 2 commits into from
Dec 18, 2024

Conversation

greggman
Copy link
Contributor

The issue is the key is selected before the modification happens which means the devices in the pool will not match the key.

I can't think of a way to fix this using the adaptor so maybe this hacky solution works by letting you modify the key.

Generally you just append a string but I made it function because MaxLimitsTestMixin wants to be able to chain to other modifiers.

Maybe it should all be re-designed

This came up because I notice a test passing that shouldn't have passed. It passsed because a MaxLimitsTestMixin test ran first and it's key was '' which meant that other tests got a max limits device from the pool.

@greggman greggman requested a review from kainino0x December 18, 2024 01:50
Copy link
Collaborator

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

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

The whole adapter/device init system could definitely use rework someday but for now I'm totally fine with piling in minor hacks, this is not that bad at all.

src/webgpu/gpu_test.ts Outdated Show resolved Hide resolved
The issue is the key is selected before the modification happens
which means the devices in the pool will not match the key.

I can't think of a way to fix this using the adaptor so maybe
this hacky solution works by letting you modify the key.

Generally you just append a string but I made it function because
MaxLimitsTestMixin wants to be able to chain to other modifiers.

Maybe it should all be re-designed

This came up because I notice a test passing that shouldn't have
passed. It passsed because a `MaxLimitsTestMixin` test ran first
and it's key was `''` which meant that other tests got a max limits
device from the pool.
@greggman greggman force-pushed the fix-descritor-modifier branch from 1f3ac76 to dda7198 Compare December 18, 2024 13:33
@greggman greggman enabled auto-merge (squash) December 18, 2024 13:35
@greggman greggman merged commit 2fdc883 into gpuweb:main Dec 18, 2024
1 check passed
@greggman greggman deleted the fix-descritor-modifier branch December 18, 2024 13:40
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.

2 participants