Add the concat algorithm by zolkis · Pull Request #366 · webmachinelearning/webnn

zolkis · 2023-03-22T20:43:31Z

First draft, but can be reviewed.
Depends on #337 and preferably the PRs preceding this.

index.bs

zolkis · 2023-05-16T15:53:04Z

@wchao1115 @huningxin PTAL
Squashed to one commit.

Squashed from the following commits: Add reference to the platform operand object Address review comment, change note. Signed-off-by: Zoltan Kis <[email protected]>

Signed-off-by: Zoltan Kis <[email protected]>

wchao1115

In your PR description, you mentioned that this is a first draft that can be reviewed, which I did here. But going forward, I'd like to suggest that we don't put a draft as a PR to prevent clogging up the PR pipeline. A PR should ideally represent a proposed change that is final in the mind of the author. This way a decision about the change can be made during the review.

wchao1115 · 2023-06-05T18:07:51Z

index.bs

+<div algorithm=rank class=algorithm-steps>
+    1. Return the size of |operand|.{{MLOperand/[[descriptor]]}}.{{MLOperandDescriptor/dimensions}}.
+</div>
+


I'm a bit worried that we use a public dictionary type like MLOperandDescriptor, which is part of the public API, as one of the internal slot data types for MLOperand. Just from the practicality standpoint if we need to extend this type in the future with additional info about an operand, we'll then need to reconcile that change with the usage as an internal slot type.

Also from the historical standpoint, the term descriptor is typically used to describe an input characteristic of something to be created. In this sense, the descriptor may actually contain info that may be ignored (e.g. non-binding hints, etc.) So, it's a bit strange to see it used as an internal slot, especially as a ground truth of something post-realized. It might be better to define a pair of slots as a dimension and a data type field separately.

But then one could say if the MLOperandDescriptor is changed, then the link between that and the internal slot is lost, so it would be even harder to track the changes...
It is quite common to have internal slots of dictionary types defined in the spec.
BTW I have actually started with what you suggest, but ended up with this since it's simpler.
@huningxin any opinion?

wchao1115 · 2023-06-05T20:11:56Z

index.bs

+<div class=algorithm-steps>
+    1. If |operand|.{{MLOperand/[[builder]]}} is not an instance of {{MLGraphBuilder}}, return `false`.
+    1. If |builder| is not `undefined` and is not equal to |operand|.{{MLOperand/[[builder]]}}, return `false`.
+    1. Let |desc| be |operand|.{{MLOperand/[[descriptor]]}}.


Related to my above comment, you make an assumption here that MLOperandDescriptor is a pair of dimension and data type. What if it changes in the future? Say having an extra field that an operand may ignore?

Then we likely need to update the algorithms anyway.
Having an extra field that is ignored is backward compatible.

wchao1115 · 2023-06-05T20:19:24Z

index.bs

 </div>

-### The concat() method ### {#api-mlgraphbuilder-concat}
+### The {{MLGraphBuilder/concat()}} method ### {#api-mlgraphbuilder-concat}


This kind of stylistic, token semantic change should not happen in isolation i.e. just look above this method at the clamp method. If we approve this change, the concat method section header will then look different from the rest. Please roll back this style change.

For more details on contributing guidelines, please look at the guidelines for contributions (CONTIBUTING.md).

We need to decide that during the stylistic change, which style to use with all titles.
This one is clickable, but for the polymorphic functions we have a problem, so it's indeed better to remove this kind of styling.

The contributing guidelines states that stylistic change among other types of change should be done atomically to reduce the chance of leaving a transient state of a change in the published spec. The problem of fragmenting what should have been a single stylistic change across many PRs is that it breaks the review process and clog the PR pipeline with related changes that aren't fully sequenced. You mentioned in your reply that this PR is no longer a draft b/c it happened many months ago, but I no longer have the context of each of your PRs whether which is before which or what state each PR is in, draft or real.

In general, the PR pipeline is much easier to move if every single PR in the pipeline is, to the best of the author's ability, atomic and that it leaves no state unfulfilled after its merge. This is what the contributing guidelines is trying to regulate.

wchao1115 · 2023-06-05T20:21:08Z

index.bs

 };
 </script>
-<div algorithm=concat>
+<div class="note">


Unclear why this is now a note class element as opposed to an algorithm identified div section.

It's a descriptive section and it is not algorithm.
If it's not a note, then its plain text, but definitely not an algorithm.

I can remove the note-ness and leave this as plain text describing the method arguments. It is not part of an algorithm.

If that is fine, then I can do this change for all the other places as well.

zolkis · 2023-06-05T21:05:54Z

In your PR description, you mentioned that this is a first draft that can be reviewed, which I did here. But going forward, I'd like to suggest that we don't put a draft as a PR to prevent clogging up the PR pipeline. A PR should ideally represent a proposed change that is final in the mind of the author. This way a decision about the change can be made during the review.

That was the first comment - it was a draft on March 22. Today is June 6. You first reviewed it on April 13, so I don't get why is this an issue now?
3 weeks ago I squashed all commits into one and requested review, which is clear indication that the PR is not a draft any more. Also, this PR was advertised before the last 3 group calls as one that should be reviewed.

wchao1115 · 2023-06-05T22:38:25Z

That was the first comment - it was a draft on March 22. Today is June 6. You first reviewed it on April 13, so I don't get why is this an issue now?

This is a meta problem that I have with the state of all of your PRs. I simply have no context of which of your PR leaves what state of change in the doc once merged. I have no recollection of my own feedback to a stylistic change that has not been merged. This is b/c my baseline for comparison is always the current state of the doc already merged in main, and not to my own previous feedback that may not have been addressed. It's most likely, however, that I will still remember my own feedback regarding API semantic changes, but highly unlikely on stylistic ones.

zolkis · 2023-06-06T07:48:34Z

I simply have no context of which of your PR leaves what state of change in the doc once merged.

Right, that needs to be clarified.
The purpose of these open PRs has been to figure out details of how to specify (dozens of) missing algorithms for the API methods. This set of PRs was selected for review because it represents a good training set for mapping what needs to be done and then extended/scaled up to a lot more coming PRs.

Here's what we learned so far:

there will be new sections for algorithms, i.e. normative steps
there will be new titles
there are polymorphic functions, with separate algorithms and titles (style TBD, by default the current plain text)
part of the argument descriptions go into describing IDL dictionary members
existing descriptive text about arguments can be kept in their own section(s), be it plain text or note (visuals being the main difference, so style is TBD).
for better readability of the sizable amount of new text, I suggested using stylistic elements and interactions.

You have expressed guidance to separate stylistic changes from fixes from new content, and created the contribution guidelines. All fine.

One thing is not possible: apply stylistic changes first, to something that does not exist yet. The current text doesn't have those parts all the stylistic changes can be applied to.

So it's a good question how to manage the transition, because during the process, as PRs get merged:

some methods will have algorithms, and some will not
the already changed parts might have reorganized method argument descriptions
which brings some basic stylistic impact, e.g. moving argument descriptions, new sections, etc.
yet I don't think it's good idea to dump all 50-60 algorithms in one PR.

So we must accept some kind of transitory process. It is up to us if this takes weeks, or years. My wish is that we move as fast as possible, let's get over it and make way and time for more meaningful contributions. Surely it won't be perfect, but after we're done, we will make a final sweep or two and iron out the wrinkles to make the spec consistent.

With the open PRs I agree, let's aim minimal changes, e.g. I will remove stylistic changes as much as possible, and if you find any residual ones that missed my eyes, for instance in titles, don't take it as intentional: I will fix those.

Signed-off-by: Zoltan Kis <[email protected]>

zolkis · 2023-06-13T15:05:28Z

Change requests have been addressed.
The PR is moved to a new target branch and is continued in #398.

wchao1115 requested changes Apr 12, 2023

View reviewed changes

index.bs Outdated Show resolved Hide resolved

anssiko mentioned this pull request Apr 13, 2023

Use modern WebIDL and Infra standard conventions #210

Closed

zolkis changed the title ~~WiP: Add the concat algorithm~~ Add the concat algorithm Apr 13, 2023

zolkis requested a review from wchao1115 April 20, 2023 13:37

zolkis force-pushed the concat-algorithm branch from 73d46fa to 387f7df Compare May 16, 2023 15:52

anssiko requested a review from huningxin May 24, 2023 06:29

Zoltan Kis added 2 commits May 25, 2023 19:14

Add the concat algorithm

7082ee7

Squashed from the following commits: Add reference to the platform operand object Address review comment, change note. Signed-off-by: Zoltan Kis <[email protected]>

Add steps for 'rank', 'validate MLOperand'. Fix the concat() steps

1651b73

Signed-off-by: Zoltan Kis <[email protected]>

zolkis force-pushed the concat-algorithm branch from 387f7df to 1651b73 Compare May 25, 2023 16:27

wchao1115 requested changes Jun 5, 2023

View reviewed changes

Zoltan Kis added 2 commits June 6, 2023 11:12

concat: remove style from title

bce29de

Signed-off-by: Zoltan Kis <[email protected]>

concat: remove note styling from argument description

2845074

Signed-off-by: Zoltan Kis <[email protected]>

zolkis mentioned this pull request Jun 13, 2023

Add algorithm to concat() #398

Merged

zolkis closed this Jun 13, 2023

zolkis mentioned this pull request Jun 22, 2023

Add internal slots to MLOperand, MLActivation and basic algorithms #336

Closed

zolkis deleted the concat-algorithm branch November 12, 2024 13:56

Comments

Conversation

zolkis commented Mar 22, 2023 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

zolkis commented May 16, 2023

Uh oh!

wchao1115 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wchao1115 Jun 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zolkis Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zolkis commented Jun 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wchao1115 commented Jun 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zolkis commented Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zolkis commented Jun 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zolkis commented Mar 22, 2023 •

edited by pr-preview bot

Loading

wchao1115 Jun 5, 2023 •

edited

Loading

zolkis Jun 6, 2023 •

edited

Loading

zolkis commented Jun 5, 2023 •

edited

Loading

wchao1115 commented Jun 5, 2023 •

edited

Loading

zolkis commented Jun 6, 2023 •

edited

Loading