Skip to content

Conversation

@AyoubMDL
Copy link
Contributor

@AyoubMDL AyoubMDL commented Aug 2, 2025

Allows passing a node name when defining a rewrite. fixes #2435

@AyoubMDL
Copy link
Contributor Author

AyoubMDL commented Aug 2, 2025

Hi @justinchuby, @gramalingam ,
In this PR, I’ve extracted the node name so it can be explicitly set when writing a rewrite. Let me know if you’d like me to apply the same approach to the other "op" parameters as well.
Also, should I make the same change in onnx_ir ?

@github-project-automation github-project-automation bot moved this from Todo to Done in ONNX Script Review Board Aug 2, 2025
@justinchuby
Copy link
Collaborator

Thanks!

the same approach to the other "op" parameters as well.
Also, should I make the same change in onnx_ir ?

What are the other parameters? What would be the change to onnx ir?

@codecov
Copy link

codecov bot commented Aug 2, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.37%. Comparing base (d0fb218) to head (11a4350).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2474      +/-   ##
==========================================
+ Coverage   70.36%   70.37%   +0.01%     
==========================================
  Files         218      218              
  Lines       26412    26428      +16     
  Branches     2646     2646              
==========================================
+ Hits        18584    18598      +14     
- Misses       6922     6923       +1     
- Partials      906      907       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AyoubMDL
Copy link
Contributor Author

AyoubMDL commented Aug 2, 2025

What are the other parameters?

overload: str = "",
graph: ir.Graph | None = None,
doc_string: str | None = None,
metadata_props: dict[str, str] | None = None,
output: ir.Value | None = None,

I am not sure if output and graph should be added.

What would be the change to onnx ir?

There is the same Builder class https://github.com/onnx/ir-py/blob/fced9bd53e7830d28d0100fb86342d3b578889ec/src/onnx_ir/_tape.py#L177

@justinchuby
Copy link
Collaborator

I see. The builder class in onnx-ir is not exposed yet. I want to reason about it's api and verify usage in onnxscript first. So far it's only used in the rewriter. (So no need to update onnx-ir for now)

@AyoubMDL AyoubMDL force-pushed the rewriter_set_op_name branch from 6eaa0b7 to eaab5ab Compare August 10, 2025 20:37
@AyoubMDL AyoubMDL force-pushed the rewriter_set_op_name branch from eaab5ab to be1776d Compare August 11, 2025 19:00
@justinchuby
Copy link
Collaborator

Thank you!

@justinchuby justinchuby enabled auto-merge (squash) September 5, 2025 21:22
@justinchuby justinchuby merged commit 9036fab into microsoft:main Sep 5, 2025
31 of 32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

[Rewriter] Cannot create an op with a name

2 participants