Add the constant() method steps.

[zolkis]

Add the constant() algorithms.
This another take on how to specify polymorphic functions (compare that with clamp() in #348
I like this approach more.

---

Preview | Diff

[dontcallmedom]

use bikeshed autolinks & definitions rather than handcrafted ids

[huningxin]

Does it make sense to pick the dependent definitions of #337, such as MLOperand internal slots and create MLOperand algorithm definitions, and merge them into this PR?

That would share a clear usage of the internal slots and algorithm definitions for the interface method steps. And that hopefully would make the PR easier for review.

WDYT?

[zolkis]

Well, all of the other PRs depend on #337, so I suggest we review and merge #337 first. Then I can rebase.
It has been "tested" by so many PRs, including polymorphic functions / activations, so it's quite stable from my p.o.v.
It is also very hard to add new algorithms with these dependencies still hanging out. I need to test everything in a separate integration repository.
For me it doesn't make much sense to pull #337 in every other PR and leave it open, since it will create a merge hell if I need to touch that part elsewhere.

What I suggest is to clean up and merge the open PRs from bottom up. Let's try to clean up the PRs that are open now, sync-async exec, operand-activation-slots, input, constant, clamp, concat, batch norm.

I will update them today with the suggestions above (factor out more algorithms), but then we should clean up and merge these.

In a week I need to publish PRs for the rest of the algorithms, but will hold them until we could merge the ones currently open.

[huningxin]

@zolkis

Well, all of the other PRs depend on #337, so I suggest we review and merge #337 first.

That's actually the issue, at least to me. It means that I cannot ensure #337 is correct before I review all the other PRs depending on it.

What I suggest is to clean up and merge the open PRs from bottom up

Instead, I feel a top-down approach might be helpful.

For example, in this PR, MLGraphBuilder.constatnt() steps clearly need to validate MLOperandDescriptor, create MLOperand and set platform operand to its internal slot. So, if these internal slot and algorithm definitions are included in one PR, it would make the review to be self-contained. Once approved, it can be merged without any dependencies. That would give you a good base for other MLOperand interfaces, like MLGraphBuilder.input(), instead of chaining all these PRs together.

Of course, I am open to other reviewers' opinion.

[zolkis]

I can do that. But all other PR's are also dependent on #337. Then I'd need to pull #337 in each and every one of them? Actually not all of #337, since only parts are needed, so that would take apart #337, which is now integral (shows in one piece the needed changes on MLOperand and MLActivation) and easy to review.

I think the morale is that people prefer self-contained PRs, even if they also touch other parts of the spec (which I wanted to avoid, also based on feedback on some earlier PRs).

[huningxin]

Then I'd need to pull #337 in each and every one of them?

I don't think so. I suppose with initial several method steps PRs merged, most of the #337 should be absorbed and merged. For example, once this MLGraphBuidler.constant() PR with its deps merged, I don't expect you need #337 for MLGraphBuilder.input() any more. I expect similar situation for operators.

IMO, the internal slots and algorithms should be defined only if they have a clear use case, e.g. the interface method steps use them. The "top-down" approach would hopefully make this very clear.

[zolkis]

IMO, the internal slots and algorithms should be defined only if they have a clear use case, e.g. the interface method steps use them.

Since I have prepared a lot of PRs in the background, I know for sure they have a clear use, and I wanted to group them in a clear way into a single PR, especially when that has earlier been encouraged.

But I do understand that for a reviewer the broken dependencies in PRs is not acceptable, and also that merging a claimed dependency just based on trust that all of it will be needed is not necessarily a good practice :).

Anyway, everything doesn't have to be perfect now and we might touch those internal slots later, too.

[zolkis]

@huningxin I pulled the dependencies and removed the unneeded ones. Now the bs builds clean, but I expect merge hell when #337 and especially #329 is merged. We should preferably merge those first.

[huningxin]

@zolkis , thanks for adding the dependent internal slots and algorithms. It makes the review much easier.

MLGraphBuilder.constant(descriptor, bufferView) looks good to me.

Left two questions for MLGraphBuilder.constant(value, type), please take a look.

[huningxin]

LGTM, thanks @zolkis !

[zolkis]

@wchao1115 : concerns addressed, let me know if there's more to do. Thanks.

[wchao1115]

I'm a bit confused by the styling of the section name. For example, here you have the constant() method name of section 7.6.2 as a quoted string, which produces a monospaced display text in the content, while all the existing sections e.g. the subsequent one 7.6.3 having the batchNormalization name outside of the quote. But then the sections before it have a mixture of quoted and unquoted method names, etc.

This is the reason behind calling out making a stylistic change a separate PR in our Contribution Guidelines. We have a lot of content in the spec, if a PR comes in with both content and styling changes, we may have missed something during the reviews and caused the document to become inconsistent styling-wise.

My recommendation for this PR is to separate out styling changes into a separate PR as described in the guidelines, or actually make them fully consistent style-wise.

[zolkis]

Thanks for taking a look.

If you check this thread, earlier we discussed why the back quotes are used. The constant() method is polymorphic. It has 2 forms like {{MLGraphBuilder/constant(descriptor, bufferView)}} and {{MLGraphBuilder/constant(value, type)}}.
Since neither is used in the title of the main section, the simple back quoted constant is used there. This is consistent over my subsequent changes.

For other methods the autolinking definition should be used.

To apply first the stylistic changes, means a lot of unwelcome manual merge for me. In addition, some of the sections do not even exist yet to which the stylistic changes will be applied. So we'd apply first those stylistic changes that apply to the current version of the document, then those which apply to the additional sections. So anyway we can't avoid doing later stylistic changes, but we can make it extremely complicated for the rules' sake.

[zolkis]

The reason we agreed to do a few PRs first was exactly to figure out consistent stylistic changes across these selected features, which will then be applied to all subsequent PRs. I mentioned in the last call that this makes the currently open PRs a bit of an exception. We need them to figure out the rules for the single-shot stylistic PR you've been asking for.

Otherwise we can't avoid going back and forth - which is okay for a few exceptions but we want to get it right before scaling up.

So if the batchNorm PR differs, that is its bug, and I will correct that. By the time we merge these PRs we will have the rules pretty much agreed upon. There are not so many of them, anyway.

[zolkis]

So here we could agree that in similar situation what style should be used.
@wchao1115 feel free to make a proposal.

[wchao1115]

The constant() method is polymorphic. It has 2 forms like {{MLGraphBuilder/constant(descriptor, bufferView)}} and {{MLGraphBuilder/constant(value, type)}}.

Thank you for clarifying. Personally, this nuance in defining a different styling for the section name based on whether the method is polymorphic is too subtle for me to anticipate, but my eyes quickly spot this visual inconsistency when I look at the section list. At the end of the day, visual quality is subjective. So I'll index more on content readability with minimal styling exceptions and special cases. I think if you can revert it to what it was before, that would be great.

[zolkis]

I have removed the back quotes from the title and made a note to self to record that preference.

[huningxin]

@zolkis , could you please squash the 18 commits and edit the commit message before merging? Thanks!

[zolkis]

@huningxin done the squash + editing, force pushed. History lost for good. :)

[huningxin]

Looks good. Thanks much @zolkis . I am going to merge it.