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

Add Status of this document note for Candidate Recommendation #340

Merged
merged 8 commits into from
Mar 31, 2023

Conversation

anssiko
Copy link
Member

@anssiko anssiko commented Feb 9, 2023

Related #322


Preview | Diff

Copy link
Contributor

@dontcallmedom dontcallmedom left a comment

Choose a reason for hiding this comment

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

I assume merging this depends on the decision on the timing of potentially merging #322?

index.bs Outdated
Status Text: <p class="note" role="note">
Further implementation experience and user feedback is being gathered for the
{{MLCommandEncoder}} interface that proposes to enable more efficient WebGPU
interoperability and the related proposal to
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not #322 is directly related to WebGPU interop, so I think that needs rephrasing.

Also, I know we've adopted the phrase "WebGPU interoperability" but I think "WebGPU integration" is a better description of what we're looking for feedback on.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, s/interoperability/integration might be better and consistent with the charter text too.

I want to hear @wchao1115's proposal how to phrase this before updating this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed with @dontcallmedom. I don't think #322 is related to WebGPU interop, or more specifically the MLCommandEncoder. This text is a bit misleading.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would you prefer to exclude MLCommandEncoder from this note entirely? Feel free to propose a better wording. We want this text to be clear.

Copy link
Collaborator

@wchao1115 wchao1115 Feb 9, 2023

Choose a reason for hiding this comment

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

The note on MLCommandEncoder should remain as we do need more data from implementation experience. We also anticipated that the WebGPU folks will have substantive feedback on it.

But it's a different topic from #322 and I think it's best to keep them separate to avoid confusing people.

re: #322, in my view it's a bit strange to ask for an implementation feedback on something that is proposed to be removed from the spec, rather than something to be added. The proposed removal of the internal GPU device would, by definition, require no implementation for it.

As a thought exercise, I think it seems far more intuitive to remove this feature from the spec, add it as an experimental feature, then ask the developers for their feedback (of implementing it).

But that's just a thought exercise to illustrate the point. I'm not proposing that we do that. I'd much prefer that we just remove the feature from the spec for good as in my mind it's a broken feature.

Copy link
Member Author

Choose a reason for hiding this comment

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

The next iteration is up for your review. Please take a look and suggest further refinements as needed. It helps if we can be concise while not oversimplifying things so improvements in that regard are especially welcome.

Copy link
Member Author

Choose a reason for hiding this comment

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

@wchao1115 PR updated with a more concise version of this note, PTAL!

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, thanks!

@anssiko
Copy link
Member Author

anssiko commented Feb 15, 2023

@dontcallmedom does the updated PR look good to you too?

@dontcallmedom
Copy link
Contributor

@anssiko can you review 8439546 before I start preparing the document for its CR publication, following w3c/transitions#491 (comment)

@anssiko
Copy link
Member Author

anssiko commented Mar 24, 2023

@dontcallmedom I submitted one comment.

(As an author of this PR I cannot approve it via GH, but you can consider this approved.)

@dontcallmedom
Copy link
Contributor

@dontcallmedom I submitted one comment.

Sorry, not seeing it? (or not finding it)

index.bs Outdated
@@ -28,6 +28,13 @@ Status Text: <p class="note" role="note">
further improvements are expected to be reflected in revised Candidate
Recommendation Drafts and Snaphots.
</p>
<p>Before requesting transition to <a href="https://www.w3.org/standards/types#PR">Proposed Recommendation</a>, the Working Group will seek to demonstrate that:</p>
<ul>
<li>the API is implementable on top of existing major platform APIs, such as Android Neural Networks API, Windows DirectML, and macOS/iOS Metal Performance Shaders and Basic Neural Network Subroutines;</li>
Copy link
Member Author

Choose a reason for hiding this comment

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

One excess whitespace in is implementable.

The substance of this line is in itself consistent with the current charter scope.

Should any of these major platform APIs be superseded with or abstracted out by new platform APIs by their owners, I expect the WG to be able to use such new platform APIs for this demonstration. I'm not sure if this detail needs to be explicitly recorded in the exit criteria or whether it can be considered common sense. Any of these platform APIs could also be (in theory) deprecated at any time.

What should be allowed is to be able to use the most appropriate API available on each platform at the time of the PR transition. I expect the implementation-experience phase we're now entering to help inform this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

given the discussion in https://github.com/webmachinelearning/webnn/pull/362/files#r1145693621, should "macOS/iOS Metal Performance Shaders and Basic Neural Network Subroutines" be replaced with ML Compute?

I agree with you that the list can only be reasonably read as informative; but if we already know better, we should use the best of our knowledge.

Copy link
Member Author

Choose a reason for hiding this comment

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

To my understanding the more recent ML Compute uses MPS and BNNS so it could be an alternative implementation path on Apple platforms. I’d prefer wording that allows Apple’s ML experts to use any of these platform APIs for their possible implementation and let that be considered another implementation in this context.

index.bs Outdated Show resolved Hide resolved
@dontcallmedom dontcallmedom marked this pull request as draft March 27, 2023 15:35
@dontcallmedom dontcallmedom marked this pull request as ready for review March 31, 2023 06:32
@dontcallmedom dontcallmedom merged commit 77f1aca into main Mar 31, 2023
@dontcallmedom dontcallmedom deleted the cr-sotd branch March 31, 2023 06:36
github-actions bot added a commit that referenced this pull request Mar 31, 2023
SHA: 77f1aca
Reason: push, by dontcallmedom

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

None yet

4 participants