-
Notifications
You must be signed in to change notification settings - Fork 48
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
Bugfix: Drop "re-throw" in MLActivation creation steps #619
Bugfix: Drop "re-throw" in MLActivation creation steps #619
Conversation
The "create an MLActivation" steps don't throw, so there's no need to re-throw. Discussed in webmachinelearning#591 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with two questions:
https://www.w3.org/TR/webnn/#create-an-mlactivation has a step to run init-steps if it presents
If init-steps are given, run init-steps with options.
I am not sure whether it would throw. However, it looks like there are no any call sites that supply the init-steps, should we remove this step accordingly?
The following step:
Otherwise, initialize activation.[[operator]] given options in an implementation-defined way for the given name operation.
I am not sure whether this step would fail. However, as #606 drops the platform operand and operator, should we drop this step as well?
I don't know the context around init-steps. Are there plans to add them? Or was it an idea that turned out not to be needed? Either way - can either merge this and follow up, or I can revise - either way, this is not blocking anything else. |
I suppose the latter. I dig the commits, and found it was introduced by @zolkis in 30ba12e. I try to recall and it seems to be intended to run the activation initialization steps supplied by different activation creation caller. But it turns out there are no callers using it. @zolkis , please correct me if I am wrong.
This PR proposes to remove the "re-throw" because "create an MLActivation" steps don't throw. I think we may need to ensure all "create an MLActivation" steps won't throw. So, if "init-steps" is useful and may throw, the "re-throw" may need tol be kept. Otherwise, the "init-steps" should be removed and that makes "create an MLActivation" steps don't throw at all, then the "re-throw" could be removed. WDYT? |
Also, simplify the use of options and operator, aligning with how MLOperands are created.
Makes sense. I removed the steps in c6ee63a, but if we decide to keep them or re-add them then we can abandon this PR and do one instead that ensures all invocations re-throw, and make it explicit that init-steps themselves may throw and that methods that invoke MLActivation creation should re-throw. |
Back then we wanted to keep optional init steps around until we figured how we end up specifying other builder algorithms. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
SHA: 2f32429 Reason: push, by huningxin Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…activation-vending SHA: 2f32429 Reason: push, by zolkis Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
The "create an MLActivation" steps don't throw, so there's no need to re-throw.
Also, nothing specifies init-steps so drop those, and simplify how the steps handle options, to align with how things are done for
MLOperands
.Discussed in #591 (comment)
Preview | Diff