MlContext improvements #310

zolkis · 2022-12-08T17:55:07Z

Several small commits to improve the createContext() steps, and add the validate MLContext steps.
Fixes #308.

Preview | Diff

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

…ate the createContext() steps. Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

zolkis · 2022-12-08T18:03:21Z

@anssiko @huningxin @wchao1115 PTAL - thanks.

After review, commits can be squashed into one.

anssiko

Thanks! One suggestion.

index.bs

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

index.bs

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

… 'create context' steps Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

zolkis · 2022-12-09T20:28:46Z

Note: for some reason the last commit (createContextSync) didn't appear in the PR, even though it was present on my branch. github didn't find that commit when clicking on its link, giving a 404 - so something went wrong.
Needed to amend (with same message), and force-push: that worked.

wchao1115 · 2022-12-09T22:48:57Z

I'm sorry @zolkis what problem we're trying to address here? The issue #308 doesn't have any detail on it.

I also forgot about the agreement around createContextSync. Is that what we previously agreed to? Adding @huningxin as a reviewer also.

huningxin · 2022-12-10T03:21:40Z

@wchao1115

I also forgot about the agreement around createContextSync. Is that what we previously agreed to?

The sync version of create context method was there since it was introduced by #149 before. Later #274 introduced the async create context method and the sync version was restricted to be exposed to dedicated worker. According to the TAG's feedback, the sync version was appended with the "Sync" postfix as createContextSync().

zolkis · 2022-12-10T08:30:26Z

I'm sorry @zolkis what problem we're trying to address here? The issue #308 doesn't have any detail on it.

Thank you for taking a look (looks like I can only mention people in PRs, and could not assign reviewers myself).
The need to validate MLContext came up in #309 (which needs to be updated when/if this PR is merged).
In addition, it is used in createContext() and its sync version. In principle, we need it in all methods where context is an argument.

These are still just editorial changes, since the steps leave the details to implementations completely. To be checked later if that needs improvement - until then, this is just a formal placeholder, marking the places implementations should do a check.

zolkis · 2022-12-12T08:00:42Z

Accidentally removed @huningxin from reviewers, but can't add back.
@anssiko have your concerns been addressed?

anssiko · 2022-12-12T08:55:45Z

Accidentally removed @huningxin from reviewers, but can't add back.
@anssiko have your concerns been addressed?

Ping me here when you've addressed the review comments and I'll add the reviewers back.

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

anssiko

LGTM. This editorial change looks good to me. Thanks for all the iteration to establish the conventions :-)

anssiko · 2022-12-12T12:01:58Z

@huningxin @wchao1115 this editorial update is now ready for your review.

You may find the visual PR Preview and Diff helpful:

Preview | Diff

index.bs

…tion section. Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

zolkis · 2022-12-13T08:13:11Z

Thank you for the review/comments, they have been addressed, and it's better. :)

anssiko · 2022-12-13T13:39:46Z

index.bs

 ### Permissions Policy Integration ### {#permissions-policy-integration}

 This specification defines a <a>policy-controlled feature</a> identified by the
 string "<code><dfn data-lt="webnn-feature">webnn</dfn></code>".
 Its <a>default allowlist</a> is <code>'self'</code>.

+### The {{ML/createContext()}} method ### {#api-ml-createcontext}
+The {{ML/createContext()}} method steps are:
+1. If [=this=]'s [=relevant global object=]'s [=associated Document=] is not [=allowed to use=] the [=webnn-feature|webnn=] feature, return [=a new promise=] [=rejected=] with a "{{SecurityError}}" and abort these steps.


"SecurityError" DOMException

Another convention round.
SecurityError is a DOMException by definition.
So, using the linked form

{{SecurityError}}

without the quotes and without DOMException it would unambiguously refer to a "SecurityError" DOMException :)

However, even if not the shortest, we should stick to the Web IDL convention:

Simple exceptions can be created by providing their error name. A DOMException can be created by providing its error name followed by DOMException. Exceptions can also be thrown, by providing the same details required to create one.

These rules are not used uniformly throughout the spec.
The question is whether to fix that in this PR, or should it be a separate one, plus document the reasoning in an issue that links this discussion?

anssiko · 2022-12-13T13:40:54Z

index.bs

+        1. Set |context|.{{[[contextType]]}} to "[=default-context|default=]".
+        1. If |options|["{{deviceType}}"] [=map/exists=], then set |context|.{{[[deviceType]]}} to |options|["{{deviceType}}"]. Otherwise, set |context|.{{[[deviceType]]}} to "[=device-type-cpu|cpu=]".
+        1. If |options|["{{powerPreference}}"] [=map/exists=], then set |context|.{{[[powerPreference]]}} to |options|["{{powerPreference}}"]. Otherwise, set |context|.{{[[powerPreference]]}} to "[=power-preference-default|default=]".
+1. If the <a>validate MLContext</a> steps given |context| return `false`, [=reject=] |promise| with a {{NotSupportedError}} and abort these steps.


"NotSupportedError" DOMException

anssiko · 2022-12-13T13:41:10Z

index.bs

+1. If [=this=]'s [=relevant global object=]'s [=associated Document=] is not [=allowed to use=] the [=webnn-feature|webnn=] feature, throw a {{SecurityError}} and abort these steps.
+1. Let |options| be the first argument.
+1. Let |context| be the result of running the <a>create context</a> steps given |options|.
+1. If the <a>validate MLContext</a> steps given |context| return `false`, throw a {{NotSupportedError}} and abort these steps.


"NotSupportedError" DOMException

anssiko · 2022-12-13T13:47:18Z

@zolkis we want to add the missing DOMException to a few places. If you search the spec with "DOMException" you'll find a few occurrences outside the scope of this PR (7.3 and 7.9) where we need to add quote marks to the error names. You can do that in a separate PR, open an issue for that editorial tweak.

zolkis · 2022-12-13T14:16:52Z

I created #314 to document the rule, but I can fix it in this PR as it's quite short, and overlaps with this PR.

In the end, the separate commits should be visible, i.e. the commit fixing the issue should not be squashed.

zolkis · 2022-12-13T14:30:18Z

The change is done, should I commit it here, or stash it and use it in another PR after this one is merged (as it is for now)?

anssiko · 2022-12-13T15:08:00Z

I suggest commit changes to the lines touched by this PR and leave the full spec error conventions update for another PR.

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

zolkis · 2022-12-13T15:45:31Z

Ah, the most complex way. It's too late to do that cleanly, I will have a merge commit on the new PR. But it's done now.

anssiko

We're almost done here :-) My proposal is to fix the error usage in createContext() and createContextSync() in this PR so we can land this.

It looks like other review feedback has been addressed, so I'll try to commit the remaining fixes to this branch and merge this PR so we can save precious Editors' review time for the upcoming PRs.

Thanks for your review @huningxin @wchao1115 and @zolkis for crafting this PR!

index.bs

'Always favor the "SyntaxError" DOMException notation over just using SyntaxError to refer to the DOMException.' This notation applies to the following error names: https://webidl.spec.whatwg.org/#idl-DOMException-error-names

SHA: 1a8de89 Reason: push, by anssiko Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

zolkis · 2022-12-13T18:48:37Z

Thanks!

zolkis · 2022-12-13T18:52:08Z

In some other projects, before merging, the smaller related commits are squashed together, to result in more meaningful history. However, we can also keep full commit history.

zolkis added 4 commits December 8, 2022 19:01

Move the Permissions Policy Integration section

2f07a8f

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

Improve the createContext() steps

1063338

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

Remove the unused internal slot MLContext/[[implementation]]

b2c6b67

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

Fix webmachinelearning#308: Add the validate MLContext algorithm. Upd…

a4e3636

…ate the createContext() steps. Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

zolkis changed the title ~~Mlcontext improve~~ MlContext improvements Dec 8, 2022

anssiko requested changes Dec 8, 2022

View reviewed changes

index.bs Outdated Show resolved Hide resolved

anssiko mentioned this pull request Dec 8, 2022

Use modern WebIDL and Infra standard conventions #210

Closed

zolkis mentioned this pull request Dec 9, 2022

Improve spec prose on "exists" #311

Closed

Use 'exists' instead of 'defined' in createContext()

f5e5cd4

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

zolkis commented Dec 9, 2022

View reviewed changes

index.bs Outdated Show resolved Hide resolved

zolkis added 2 commits December 9, 2022 22:12

Reformulate 'exists' clauses in createContext()

5b9b517

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

Add the createContextSync() section and algorithm, using factored-out…

5961fd8

… 'create context' steps Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

zolkis force-pushed the mlcontext-improve branch from 523e793 to 5961fd8 Compare December 9, 2022 20:26

wchao1115 requested a review from huningxin December 9, 2022 22:48

zolkis requested review from anssiko and removed request for huningxin December 12, 2022 07:58

zolkis added 3 commits December 12, 2022 11:29

Make 'then' clauses a sub-step in createContext()

396327f

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

Re-align if clauses in createContext()

a3f3ecc

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

Use map-get convention in createContext()

eed627b

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

anssiko approved these changes Dec 12, 2022

View reviewed changes

anssiko requested review from huningxin and wchao1115 December 12, 2022 11:58

wchao1115 requested changes Dec 12, 2022

View reviewed changes

index.bs Outdated Show resolved Hide resolved

huningxin reviewed Dec 13, 2022

View reviewed changes

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

Fix review comments: false instead failure, title of MLContext valida…

6c5b11b

…tion section. Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

anssiko reviewed Dec 13, 2022

View reviewed changes

anssiko self-requested a review December 13, 2022 13:47

zolkis mentioned this pull request Dec 13, 2022

WiP: editorials #301

Closed

Fix error handling, only for this PR

fc8854c

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

anssiko requested changes Dec 13, 2022

View reviewed changes

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

Use the recommended DOMException notation

579d815

'Always favor the "SyntaxError" DOMException notation over just using SyntaxError to refer to the DOMException.' This notation applies to the following error names: https://webidl.spec.whatwg.org/#idl-DOMException-error-names

anssiko approved these changes Dec 13, 2022

View reviewed changes

anssiko merged commit 1a8de89 into webmachinelearning:main Dec 13, 2022

github-actions bot added a commit that referenced this pull request Dec 13, 2022

Merge pull request #310 from zolkis/mlcontext-improve

c458de7

SHA: 1a8de89 Reason: push, by anssiko Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

zolkis mentioned this pull request Dec 13, 2022

Fix error handling #315

Merged

zolkis deleted the mlcontext-improve branch January 18, 2023 17:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MlContext improvements #310

MlContext improvements #310

zolkis commented Dec 8, 2022 •

edited by pr-preview bot

Loading

zolkis commented Dec 8, 2022 •

edited

Loading

anssiko left a comment

zolkis commented Dec 9, 2022

wchao1115 commented Dec 9, 2022

huningxin commented Dec 10, 2022 •

edited

Loading

zolkis commented Dec 10, 2022 •

edited

Loading

zolkis commented Dec 12, 2022

anssiko commented Dec 12, 2022

anssiko left a comment

anssiko commented Dec 12, 2022

zolkis commented Dec 13, 2022

anssiko Dec 13, 2022

zolkis Dec 13, 2022

anssiko Dec 13, 2022

anssiko Dec 13, 2022

anssiko commented Dec 13, 2022

zolkis commented Dec 13, 2022

zolkis commented Dec 13, 2022

anssiko commented Dec 13, 2022

zolkis commented Dec 13, 2022

anssiko left a comment

zolkis commented Dec 13, 2022

zolkis commented Dec 13, 2022

MlContext improvements #310

MlContext improvements #310

Conversation

zolkis commented Dec 8, 2022 • edited by pr-preview bot Loading

zolkis commented Dec 8, 2022 • edited Loading

anssiko left a comment

Choose a reason for hiding this comment

zolkis commented Dec 9, 2022

wchao1115 commented Dec 9, 2022

huningxin commented Dec 10, 2022 • edited Loading

zolkis commented Dec 10, 2022 • edited Loading

zolkis commented Dec 12, 2022

anssiko commented Dec 12, 2022

anssiko left a comment

Choose a reason for hiding this comment

anssiko commented Dec 12, 2022

zolkis commented Dec 13, 2022

anssiko Dec 13, 2022

Choose a reason for hiding this comment

zolkis Dec 13, 2022

Choose a reason for hiding this comment

anssiko Dec 13, 2022

Choose a reason for hiding this comment

anssiko Dec 13, 2022

Choose a reason for hiding this comment

anssiko commented Dec 13, 2022

zolkis commented Dec 13, 2022

zolkis commented Dec 13, 2022

anssiko commented Dec 13, 2022

zolkis commented Dec 13, 2022

anssiko left a comment

Choose a reason for hiding this comment

zolkis commented Dec 13, 2022

zolkis commented Dec 13, 2022

zolkis commented Dec 8, 2022 •

edited by pr-preview bot

Loading

zolkis commented Dec 8, 2022 •

edited

Loading

huningxin commented Dec 10, 2022 •

edited

Loading

zolkis commented Dec 10, 2022 •

edited

Loading