-
Notifications
You must be signed in to change notification settings - Fork 49
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
Remove MLDeviceType #809
Conversation
There was a problem hiding this 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.
@fdwr commented:
Since enums are not web-exposed, we could leave the I created a new issue for discussing such a query mechanism: #815
@zolkis I think this should be added to the device selection explainer as a new use case. |
0550c98
to
b10fef0
Compare
* 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]>
b10fef0
to
7ce5a42
Compare
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. |
@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. |
Co-authored-by: Anssi Kostiainen <[email protected]>
Co-authored-by: Anssi Kostiainen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Per our discussion on WebML WG Teleconference – 13 February 2025, this PR is ready to merge. Thank you everyone! |
SHA: 9c00304 Reason: push, by anssiko Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
- 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]>
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