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

Remove MLDeviceType #809

Merged
merged 4 commits into from
Feb 13, 2025

Conversation

zolkis
Copy link
Collaborator

@zolkis zolkis commented Jan 27, 2025

Resolves #749.
Addresses #302, #350.
See also the device selection explainer.

This is a minimal change for now. It may require changing prose or adding more details / instructions in related sections/algorithms.


Preview | Diff

@inexorabletash
Copy link
Member

@zolkis can you use the keyword "Resolves #749" in the PR description? This creates a visible link between the issue and PR.

Also... would you consider #302 addressed by this PR?

Copy link
Member

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

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

Overall LGTM, lots of nitpicks.

@anssiko
Copy link
Member

anssiko commented Jan 30, 2025

@zolkis I believe this PR also addresses #350 by making MLContext device agnostic.

@anssiko
Copy link
Member

anssiko commented Feb 4, 2025

@fdwr commented:

Thoughts? Do we delete MLDeviceType for now and then resurrect it after deciding how to query which kinds of devices a context can support? Do keep it but remove the context creation parameter?

Since enums are not web-exposed, we could leave the MLDeviceType enum in place (for later reuse/repurpose) but remove it from the MLContextOptions dictionary. In addition could update this note to add that a mechanism to query devices a context can support is being considered.

I created a new issue for discussing such a query mechanism: #815

or it might be okay to use CPU, but the app could load a different model that's more CPU-friendly, if it knew that was the case.

@zolkis I think this should be added to the device selection explainer as a new use case.

@zolkis zolkis force-pushed the device-selection-refactoring branch 4 times, most recently from 0550c98 to b10fef0 Compare February 6, 2025 15:07
inexorabletash and others added 2 commits February 6, 2025 17:10
* Ensure object creation specifies the realm

"Realm" is an ECMAScript concept best explained in
https://html.spec.whatwg.org/multipage/webappapis.html#realms-and-their-counterparts
Newly created JS objects must be associated with a Realm; while older
specs didn't do this explicitly, best practice is to be explicit about
this, especially for steps running "in parallel", or in algorithms
separate from method steps. Do so!

This also adds lint tests to try and catch future violations. Note
that dictionaries (e.g. MLOperatorDescriptor) are Infra "ordered maps"
it the body of spec algorithms, not JS objects, so they don't have a
realm. Conversion to a JS object when returning a dictionary to script
is handled by WebIDL bindings logic.

Also note that DOMExceptions, either thrown or as promise rejection
values, are not given a realm. This is a known issue across all web
specs and is tracked in whatwg/webidl#135.

Resolves webmachinelearning#793.

* Don't double-init realm; and don't need realm for dicts

* Variable name improvement from @fdwr
Signed-off-by: Zoltan Kis <[email protected]>

Improve text on device selection

Co-authored-by: Joshua Bell <[email protected]>

Improve text on fingerprinting

Co-authored-by: Joshua Bell <[email protected]>

Improve text on underlying device selection

Co-authored-by: Joshua Bell <[email protected]>

Address review comments from Ningxin

Fix RFC2119 issue for Note

Signed-off-by: Zoltan Kis <[email protected]>
@zolkis zolkis force-pushed the device-selection-refactoring branch from b10fef0 to 7ce5a42 Compare February 6, 2025 15:11
@zolkis
Copy link
Collaborator Author

zolkis commented Feb 6, 2025

Apologies for the forced pushes. Squashed these simple changes into one commit, but was interleaved with others from the main, so it needed rebase, then reorder/merge, then squash/merge. Now it should be clean to merge.

@anssiko
Copy link
Member

anssiko commented Feb 6, 2025

@zolkis thanks for addressing the review feedback. I added a few suggestions for the open points and marked as resolved those conversations that were addressed. Please double-check.

To allow for adequate review time, let's keep this PR open until our next call, 2025-02-13.

zolkis and others added 2 commits February 6, 2025 18:51
Copy link
Contributor

@huningxin huningxin left a comment

Choose a reason for hiding this comment

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

LGTM!

@anssiko
Copy link
Member

anssiko commented Feb 13, 2025

Per our discussion on WebML WG Teleconference – 13 February 2025, this PR is ready to merge. Thank you everyone!

@anssiko anssiko merged commit 9c00304 into webmachinelearning:main Feb 13, 2025
2 checks passed
github-actions bot added a commit that referenced this pull request Feb 13, 2025
SHA: 9c00304
Reason: push, by anssiko

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
inexorabletash added a commit to inexorabletash/webnn that referenced this pull request Feb 13, 2025
- remove MLDeviceType and related prose
- update "create a context" algorithm
- be explicit MLContextOptions is a hint

Closes webmachinelearning#302, closes webmachinelearning#350, closes webmachinelearning#749

Signed-off-by: Zoltan Kis <[email protected]>
Co-authored-by: Joshua Bell <[email protected]>
Co-authored-by: Anssi Kostiainen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MLContextOptions.deviceType seems unnecessary outside of conformance testing
6 participants