Support optional attribute checking in matcher #1629
Conversation
| @@ -205,6 +205,7 @@ def __call__( | |||
| domain: str | None = None, | |||
| version: int | None = None, | |||
| outputs: int | list[str | None] = 1, | |||
| _allow_other_attributes: bool | None = None, | |||
There was a problem hiding this comment.
Does this mean the option is experimental and not user facing?
There was a problem hiding this comment.
Not really. It is more to avoid conflicts with ONNX attribute names. We discussed doing something similar for other names as well, like domain/version/outputs. Any thoughts about this? May be a good time to do it.
There was a problem hiding this comment.
I see. I don't have a better idea yet but I am wishing for one
There was a problem hiding this comment.
(Post internal discussion comment): My personal preference and recommendation is to use kwargs with a leading underscore for domain/version/outputs/ etc. This
- Meets the goal of distinguishing ONNX op attributes from builder's custom options,
- Involves the least typing effort for users and least cognitive overhead in reading-or-writing (except for using names like domain/version, which fails the first criterion).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1629 +/- ##
=======================================
Coverage 75.58% 75.58%
=======================================
Files 240 240
Lines 25653 25690 +37
Branches 4615 4621 +6
=======================================
+ Hits 19389 19418 +29
- Misses 5385 5390 +5
- Partials 879 882 +3 ☔ View full report in Codecov by Sentry. |
Test Results 30 files ± 0 30 suites ±0 1h 25m 28s ⏱️ - 5m 36s For more details on these failures, see this check. Results for commit 2b151af. ± Comparison against base commit 159e5bc. This pull request removes 1465 and adds 1 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
…-script into rama/matcher-attr
|
Should the |
|
I would also add a docstring for NodePattern |
Extend matcher to allow users to specify whether all attributes must be exactly as in pattern. Change default-value to allow extra-attributes in actual node, not specified in pattern.