Skip to content

Add validation of op registrations#5817

Merged
guoyu-wang merged 5 commits into
masterfrom
skottmckay/ValidateOpRegistrations
Nov 17, 2020
Merged

Add validation of op registrations#5817
guoyu-wang merged 5 commits into
masterfrom
skottmckay/ValidateOpRegistrations

Conversation

@skottmckay
Copy link
Copy Markdown
Contributor

Description:

Add validation of operator registrations to the reduction script

  • the script has all the logic to process the registrations, and there are multiple CI's that run it
  • KernelRegistry receives the OpSchema but that only has a start version, so we can't do the validation at that level

Fix some operator registrations

Motivation and Context
Prevent invalid op registrations in the future.

  - the script has all the logic to process the registrations, and there's a CI that uses it

Fix some operator registrations
@skottmckay skottmckay requested a review from a team as a code owner November 15, 2020 23:35
Comment thread tools/ci_build/exclude_unused_ops.py Outdated
Comment thread tools/ci_build/exclude_unused_ops.py Outdated
Comment thread tools/ci_build/exclude_unused_ops.py Outdated
key, value = entry
opset_from, opset_to = value

deprecated = key in deprecated_ops and opset_to == deprecated_ops[key] - 1
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

deprecated_ops[key] - 1 [](start = 59, length = 23)

If we do this calculation every time and there is no other dependency on deprecated_ops[key], why not we just save the targeted value there for convenience?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We do this once after processing all registrations. Not sure what you mean by 'save the targeted value there'.


In reply to: 523938771 [](ancestors = 523938771)

Comment thread tools/ci_build/exclude_unused_ops.py Outdated
Comment thread tools/ci_build/exclude_unused_ops.py Outdated
… exclude ops script and an op registration validation script.

Run op validation in minimal build CI
Copy link
Copy Markdown
Contributor

@guoyu-wang guoyu-wang left a comment

Choose a reason for hiding this comment

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

:shipit:


ort_root = os.path.abspath(args.ort_root) if args.ort_root else ''
include_cuda = True # validate CPU and CUDA EP registrations

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess this does not really matter, but it will be nice this can be a param also and default to true

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Might as well validate all the registrations. Seems an unnecessary complication to have a param to do a subset of files (vs the use case where you're disabling registrations and modifying files where it makes more sense to do that sort of thing).

@guoyu-wang guoyu-wang merged commit c84bc25 into master Nov 17, 2020
@guoyu-wang guoyu-wang deleted the skottmckay/ValidateOpRegistrations branch November 17, 2020 18:44
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.

3 participants