search

webmachinelearning / webnn

🧠 Web Neural Network API
https://www.w3.org/TR/webnn/
Other
378 stars 46 forks source link

Use modern WebIDL and Infra standard conventions #210

Closed anssiko closed 8 months ago

anssiko commented 3 years ago
ℹ️ This is a meta issue. See the (living) task list for pointers to specific issues and PRs.

The WebIDL spec recommends algorithmic method steps for defining operations: https://heycam.github.io/webidl/#method-steps

This spec currently uses a WebGPU spec inspired convention that differs from this best practice. We should consider adopting the "method steps" convention.

(Here's one example of method steps in context of the WebNN API spec: https://webmachinelearning.github.io/webnn/#dom-ml-createcontext)

anssiko commented 2 years ago

I labeled this issue with "cr" implying we should address this issue before our Candidate Recommendation publication to satisfy "The document is considered complete and fit for purpose" requirement.

anssiko commented 2 years ago

Updated patterns for defining getters, setters, methods and constructors:

https://webidl.spec.whatwg.org/#getter-steps https://webidl.spec.whatwg.org/#setter-steps https://webidl.spec.whatwg.org/#method-steps https://webidl.spec.whatwg.org/#constructor-steps

anssiko commented 2 years ago

@zolkis we discussed this issue on our call with a recommendation on tasks where to start: https://www.w3.org/2022/09/22-webmachinelearning-minutes.html#t04

This issue is tagged as "cr" so we'll allocate time to discuss this on our bi-weeklies. Please bring any open issues or questions either to this issue or to the call. There is likely a lot of details that need to be clarified as you translate the current text into an algorithmic form. Thanks for your contributions!

anssiko commented 1 year ago

(API review questions that'll also inform this work on method steps discussed in https://github.com/webmachinelearning/webnn/issues/298)

anssiko commented 1 year ago

(Renamed the issue to better reflect the scope of this work.)

@zolkis I will document here the spec sections where we need to do editorial work such as add algorithmic method steps, add internal slots, use getters, setters, define methods and constructors using the modern WebIDL and Infra standard conventions.

This helps the WG understand the scope of this effort and areas where contributions are welcome. I would focus this task on editorial changes and only propose normative changes when they are needed to fix a spec bug, or to clarify an aspect where the spec and implementation disagree.

Integration branch

January 2024 update: Review feedback status (🟡 WIP, 🟢 fixed) updated.

August 2023 update: The big PR https://github.com/webmachinelearning/webnn/pull/446 has been merged. The remaining "review feedback" issues to be addressed separately.

June 2023 update: We use the zk-conventions-integration integration branch for this conventions alignment task.

PRs related to this conventions update will be merged to this integration branch. We merge the integration branch into main when all the related issues are addressed in the integration branch to keep main in a cohesive state and changes to it atomic.

Issues and PRs

zk-conventions-integration

Global changes

MLOperand, MLActivation

MLGraphBuilder

Ops

main

Other

Proposals for normative changes that were unearthed as part of the editorial work.

zolkis commented 1 year ago

This issue is good aggregate tracker. Issues might be spanned on need, if the discussion is expected to be contained, or too big.

zolkis commented 1 year ago

I have been developing the features from the TODO in separate private branches in my fork. Some of them were published in PRs (some merged, some open). Most of them are still in private branches in my local repo. I will push them to my fork (without making PRs) so they become visible.

I was asked to present all upcoming changes in one fork/branch, so that all changes could be reviewed with the full context. The tip of my integration branch, containing all changes from the TODO above, is now visible on https://zolkis.github.io/webnn/ (index.bs and index.html are being kept up to date, rebased from main and including any upcoming fixes to my changes). The changes from the current open PRs (clamp, concat, batch norm, sync-async execution, input fix) have been included.

@huningxin : the improved steps that we worked out in the clamp PR (explicit steps to create platform objects for operands and activations) has been included. Should we next update the rest of the algorithms based on that?

@wchao1115 you can take a look on the current state, and if you have any feedback, it would be good to fix and include those as early as possible (so far you had feedback on types used in internal slots, not using note style for argument descriptions and remove styling from titles). I have fixed the latter two.

On how to make feedback: either in one or more issues, or I could make PRs to an upstream integration branch to be made for this purpose (e.g. zolkis-algorithms-integration or any other preferred name in the upstream repo) from which we could rebase/PR to main when the time comes. Could someone with access rights create that target integration branch?

wchao1115 commented 1 year ago

Thanks @zolkis. Can you please remove your current set of related PRs in the mainline PR queue, now that you have the entire change hosted in your fork?

With the expectation that the change in this fork will be scoped to

  1. A global doc style change, and
  2. Additional implementation sections as required by the convention,

the reviewers will review the entire change in the fork and once approved, merge it all at once when it's all ready to go. Any additional change after the merge is expected to be minimal and scoped to adjusting to the set of changes previously merged. Your private fork should therefore no longer be needed once the merge happens.

This request was made with the intention to provide the reviewers with enough context behind the whole change, which will unavoidably affect the entire spec as a whole and not incremental in nature.

The reviewers expect this entire change to be atomic, meaning that it will leave no follow-up action items after it is merged to the main content. It is important to remember that the change in this private fork is strictly scoped to the two types of changes noted above in order to avoid having a separate parallel content that is in the unmergeable state to the mainline content.

zolkis commented 1 year ago

@wchao1115: affected PRs will be closed when the alternative will have been set up. Right now it's not yet finished setting up.

the reviewers will review the entire change in the fork

How exactly these changes are proposed to be reviewed? Reviews need PRs. PRs need a target branch, which (if not the main branch) is currently missing. I don't have access rights to make it. I guess @anssiko can make it.

IMO the most practical way to review and integrate these changes, without affecting the main branch, is using normal github practices, as follows:

About the content of the changes: bear in mind that these are actually adding algorithms, i.e. normative part of the spec. It might result in effects on the current text (so far not much, only arguments descriptions have been moved). Stylistic changes come as needed, like adding new (sub)sections and titles, collapsible algorithm blocks, visual framing of certain blocks (internal slots, validation, algorithms, etc.) IIRC no other changes were needed, and the existing changes fit pretty well in your requested policies. Any further, deeper changes can be done later via issues and PRs in the main branch (regular spec work).

anssiko commented 1 year ago

The zk-conventions-integration integration branch is now ready for review.

These changes align the entire specification with modern specification conventions and add stylistic improvements on top that make navigating this specification more delightful experience.

The following resources are made available to the group to assist in this review task:

Thanks @zolkis for this significant effort!

wchao1115 commented 1 year ago

@anssiko and @zolkis, do we know the ETA to merge zk-conventions-integration branch to main? I've spent some good amount of time on the change, and as far as I can tell, I'm signed off on it. If there's additional change to be made, please let me know how you plan to make them, either in this staging branch, or directly targeting main as needed, and if so, by when. Thanks!

zolkis commented 1 year ago

@wchao1115 we agreed in the WG meeting to spend the time until the next WG meeting for proofreading, sanity check etc. Our implementation team helps in doing that, and I am making extra effort to handle all comments possible, and make issues for the larger or more complex changes to be handled later. The target is that by the next WG meeting, it will be in good enough shape to be merged, and then handle the rest soon after.

anssiko commented 1 year ago

@wchao1115, we expect to merge latest after our next meeting 24 Aug. We have a good momentum with reviews with an extended team pulled in to help with PR #446 review and @zolkis is focused on addressing the review comments directly in the integration branch (see commits). We open new issues for proposed changes more appropriate to be addressed after this big PR has been merged.

Thanks for your efforts in driving this to completion, everyone!

anssiko commented 1 year ago

The big PR https://github.com/webmachinelearning/webnn/pull/446 has been merged, thank you all! Your contributions have been acknowledged in the specification.

The modernized specification is now published at its official location (https://www.w3.org/TR/webnn/) as a Candidate Recommendation Draft to signal there have been significant changes to the previous published document 🚀

I'll leave this tracker issue open for a while to help the WG take to completion the few "review feedback" issues (noted in my comment above) we wanted to address on top of this big change.

anssiko commented 9 months ago

Per our discussion on today's call, I'd propose to retire this tracker issue and move to a GH label-based tracking of standards convention updates.

I've asked @inexorabletash to help me with this task. Let me know when you've absorbed the useful information from this big meta issue so we can close this.

inexorabletash commented 9 months ago

Thanks @anssiko

So far as I can tell, all of the topics mentioned here are either already fixed or are tracked by open issues. Here's how I'd categorize what's still open:

Further labels e.g. "PR needed" "has PR", "needs WPT", "has WPT" might be useful but that can be discussed separately.

anssiko commented 8 months ago

@inexorabletash much thanks for the label proposals and triage.

I think we could document the proposed labels and their semantics in a .md file alongside code conventions and contrib guidelines. An idea up for grabs, PRs welcome :)

anssiko commented 8 months ago

Thanks everyone for your work on this WebIDL and Infra standard conventions alignment effort that has resulted in a significantly improved and more precise specification. Special thanks to @zolkis and @inexorabletash for this effort!

I opened #549 and triaged the remaining open issues referenced here. We're now ready to close this tracker 👋

From now on standards conventions related issues can be found via this label: https://github.com/webmachinelearning/webnn/labels/conventions