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

Update DevicePool to provide multiple devices #1142

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

haoxli
Copy link
Contributor

@haoxli haoxli commented Apr 5, 2022

  • Remove mismatched device pool, make the device pool could provide
    multiple devices at once, and reserve a default mismatched device
    at test initialization.
  • Cancel selecting the default mismatched device in tests. For
    requesting mismatched device with particular features, we still need
    to call selectMismatchedDeviceOrSkipTestCase.

Issue: #912


Requirements for PR author:

  • All missing test coverage is tracked with "TODO" or .unimplemented().
  • New helpers are /** documented */ and new helper files are found in helper_index.txt.
  • Test behaves as expected in a WebGPU implementation. (If not passing, explain above.)

Requirements for reviewer sign-off:

  • Tests are properly located in the test tree.
  • Test descriptions allow a reader to "read only the test plans and evaluate coverage completeness", and accurately reflect the test code.
  • Tests provide complete coverage (including validation control cases). Missing coverage MUST be covered by TODOs.
  • Helpers and types promote readability and maintainability.

When landing this PR, be sure to make any necessary issue status updates.

- Remove mismatched device pool, make the device pool could provide
  multiple devices at once, and reserve a default mismatched device
  at test initialization.
- Cancel selecting the default mismatched device in tests. For
  requesting mismatched device with particular features, we still need
  to call selectMismatchedDeviceOrSkipTestCase.
@github-actions
Copy link

github-actions bot commented Apr 5, 2022

Previews, as seen when this build job started (c51c6a2):
Run tests | View tsdoc

@Jiawei-Shao
Copy link
Collaborator

Hi @kainino0x , we'd like to invite you to review this PR as it changes the test framework. Could you take a look?

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.

It's late so I didn't have time for a full review of the framework code, but one key question.

/** Device with no descriptor for device mismatch validation. */
private mismatchedDefaultHolder: DeviceHolder | 'uninitialized' | 'failed' = 'uninitialized';
/** Devices with descriptors for device mismatch validation. */
private mismatchedNonDefaultHolders = new DescriptorToHolderMap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks essentially just like having two DevicePool objects (like before), but they've been merged into one object. What's the benefit to that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for adding new properties, rather than expanding the existing holders into an array, is to make it easier to know where to delete the device when releasing a device, otherwise we need to go through all devices in the holders to check if it's default device or mismatched device.

And if change defaultHolder and holders in nonDefaultHolders to array, we use the first solt of DeviceHolders[] as the default device and the second solt as the mismatched device (so I wanted to change the mismatchedDevice to secondaryDevice before), but in terms of code reading, I think it is not very friendly to know their usages.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand why you do it as separate properties rather than an array, it's definitely a simpler change. But why do we need this change at all? It seems functionally identical to just having two DevicePools, which are more cleanly separated from each other.

I think you also made every GPUTest have two devices (primary and mismatched). That seems like it can be done just fine with two DevicePools. That said, I still don't think it's good to have every test in the entire test suite use two devices. Only the device-mismatch tests need them, and I don't think one line of setup in those tests is a problem. (Right now it's three, because of the if (mismatched) check, but that's not really necessary.)

Copy link
Contributor Author

@haoxli haoxli Apr 7, 2022

Choose a reason for hiding this comment

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

Oh! Do you meams why remove the these lines:

if (mismatched) {
  await t.selectMismatchedDeviceOrSkipTestCase(undefined);
}

I remember that you had a comment at #741, said

Come to think of it, it's also unfortunate that selectMismatchedDeviceOrSkipTestCase needs to be called in every test separately. It would be ideal if it happened implicitly, and if it wasn't necessary to pass in a device descriptor at all - the mismatched device should always have the same device descriptor as the original device

If we want to have a mismatched device implicitly, we need to initialize the device in every test, otherwise selectMismatchedDeviceOrSkipTestCase is needed in mismatched validation tests.

This looks essentially just like having two DevicePool objects (like before), but they've been merged into one object.

I'm not sure if I misunderstood the DevicePool support multiple devices here. What I throught was adding more DeviceHolders in the DevicePool (using array or more properties) or making DeviceHolder could contain more than one devices with same descripters (what I did in previous PR). Isn't this what you mean by supporting multiple devices in DevicePool?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I remember that you had a comment at #741, said

Come to think of it, it's also unfortunate that selectMismatchedDeviceOrSkipTestCase needs to be called in every test separately. It would be ideal if it happened implicitly, and if it wasn't necessary to pass in a device descriptor at all - the mismatched device should always have the same device descriptor as the original device

Ah yes, I remember that. I think the implementation I had in mind was, if a test uses the .mismatchedDevice property (or .getMismatchedDevice()) then we would get one automatically, rather than having to request one. I don't think we should do it for all tests regardless of whether they need one.

I'm not sure if I misunderstood the DevicePool support multiple devices here.

The intent of this MAINTENANCE_TODO was to generalize the DevicePool so we can run multiple tests in parallel (since tests are async). But it hasn't been important so we haven't done anything about it.

// Always attempt to initialize default device, to see if it succeeds.
let errorMessage = '';
if (this.defaultHolder === 'uninitialized') {
let defaultHolder = mismatched ? this.mismatchedDefaultHolder : this.defaultHolder;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: this isn't "default holder", should just be called holder

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.

3 participants