Update script to find optimizers that potentially need supported opset updates#12330
Conversation
…ical these days. Update to new path for the cpu contrib_op kernel registrations.
…timizerOpsetVersionCheckScript
Co-authored-by: Justin Chu <[email protected]>
|
Hi @skottmckay sorry I just left a nit passing by - would you like me to review and approve or do you have reviewers in mind? |
Thanks for the comment. I'm happy for you to review. It's a fairly simple change to a helper script we can manually run when the ONNX version is updated. |
| # look for new match | ||
| start = line.find(function_or_declaration) | ||
| if start != -1: | ||
| cur_line = line |
There was a problem hiding this comment.
Does it make sense to store from where the function starts to save the need for cur_line.find(function_or_declaration)? This way we can do cur_line[len(function_or_declaration): end] and be rid of the pyright error "find" is not a known member of "None"
There was a problem hiding this comment.
Changed to store from the '(' so a find isn't needed when appending to results.
…timizerOpsetVersionCheckScript
| if start != -1: | ||
| cur_line = line | ||
| # include the '(' in case that is the last character in the line | ||
| current_args = line[start:].strip() |
There was a problem hiding this comment.
Looks like find returns the start position of the substring. line[start:] will include function_or_declaration as well. is this intended?
>>> "abcd".find("bc")
1
There was a problem hiding this comment.
Good catch. The script works but only as the first argument wasn't used in the calling code. Reworked it to be a bit more explicit.
| have_all_args = cur_line and end != -1 | ||
| have_all_args = current_args and end != -1 | ||
|
|
||
| if have_all_args: |
There was a problem hiding this comment.
I would add assert current_args to make the type checker happy
There was a problem hiding this comment.
Reworked it so it doesn't complain.
Not a fan of random statement to make a checker happy. That imposes a maintenance cost of wasted time as each developer that sees that line wonders why there's such a meaningless check, potentially removes it as it's just noise, finds the invalid pyright complaint, and changes it back.
There was a problem hiding this comment.
Thanks! This is even better
|
This pull request introduces 1 alert when merging b7a251e into 5d1173f - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging 4837a50 into 5d1173f - view on LGTM.com new alerts:
|
| have_all_args = cur_line and end != -1 | ||
| have_all_args = current_args and end != -1 | ||
|
|
||
| if have_all_args: |
There was a problem hiding this comment.
Thanks! This is even better
Co-authored-by: Justin Chu <[email protected]>
skottmckay
deleted the
skottmckay/UpdateOptimizerOpsetVersionCheckScript
branch
2 participants
Description:
Update to handle multiline declarations for the kernels which are typical these days.
Update to new path for the cpu contrib_op kernel registrations.
Motivation and Context
Fix helper to find optimizers that need updating.