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

Synchronously validate input operands/validations #605

Merged

Conversation

inexorabletash
Copy link
Contributor

@inexorabletash inexorabletash commented Mar 16, 2024

Previously the spec had a "validate MLOperand" helper that (1) ensured the operand was from the passed MLGraphBuilder and (2) that the operand was internally consistent, and this was called during (3) build() and (4) only concat() among the vending methods.

  • (1) is needed but can be done when the MLOperand is created, giving better feedback, so (3) isn't needed.

  • (2) is not needed - MLOperands are immutable so they can't be created in a bad state.

  • (4) should be expanded to all MLOperand creations that take input MLOperands.

This renames the helper, ensures it is called by every MLOperand vending method that takes MLOperand inputs, and drops it from build(). Similar validation is added for MLActivation inputs.

For #572


Preview | Diff

Previously the spec had a "validate `MLOperand`" helper that (1)
ensured the operand was from the passed `MLGraphBuilder` and (2) that
the operand was internally consistent, and this was called during (3)
`build()` and (4) only `concat()` among the vending methods.

- (1) is needed but can be done when the `MLOperand` is created,
  giving better feedback, so (3) isn't needed.

- (2) is not needed - `MLOperands` are immutable so they can't be
  created in a bad state.

- (4) should be expanded to all `MLOperand` creations that take input
  `MLOperand`s.

This renames the helper, ensures it is called by every `MLOperand`
vending method that takes `MLOperand` inputs, and drops it from
`build()` (although that will probably collide with PR webmachinelearning#603 which
should land first). Similar validation is added for `MLActivation`s.

For webmachinelearning#572
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 with nits, thanks!

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
Missed a few "if it exists" clauses

Co-authored-by: Ningxin Hu <ningxin.hu@intel.com>
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!

Copy link
Collaborator

@fdwr fdwr left a comment

Choose a reason for hiding this comment

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

👏

@fdwr fdwr merged commit b04124c into webmachinelearning:main Mar 20, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Mar 20, 2024
SHA: b04124c
Reason: push, by fdwr

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@inexorabletash inexorabletash deleted the content-validate-input-operands branch March 20, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants