Skip to content

[IR] Introduce node convenience functions #2303

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

Closed

Conversation

Johansmm
Copy link
Contributor

Introduce insert_nodes_in_value and remove_nodes convenience functions to insert and remove nodes, respectively.

Close #2294

@Johansmm Johansmm force-pushed the introduce-node-convenience-functions branch from 8239060 to e89df33 Compare May 13, 2025 21:54
@justinchuby justinchuby self-assigned this May 14, 2025
@justinchuby justinchuby self-requested a review May 14, 2025 01:56
@Johansmm Johansmm changed the title Introduce node convenience functions [IR] Introduce node convenience functions May 15, 2025
@justinchuby
Copy link
Collaborator

Thanks - I will get to it hopefully some time this week

graph.sort()


def remove_nodes(nodes: Sequence[_core.Node]) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is very useful, thanks

Copy link
Collaborator

@justinchuby justinchuby May 30, 2025

Choose a reason for hiding this comment

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

Maybe call it remove_connected_nodes ? Also could you move this PR to https://github.com/onnx/ir-py now that we finished migration? (sorry about the extra effort) I recommend creating two PRs for the two functions so they can be reviewed individually

@Johansmm
Copy link
Contributor Author

@justinchuby, I forgot to ask something: when you finish reviewing, I would like to discuss with you about the implementation and see the possibility of updating replace_nodes_and_values under the same logic (considering renaming it to replace_nodes).

@justinchuby
Copy link
Collaborator

Sure - happy to explore. I may need a few more days to review this PR. I am occupied with some other tasks at the moment

@Johansmm
Copy link
Contributor Author

Sure - happy to explore. I may need a few more days to review this PR. I am occupied with some other tasks at the moment

Do not worry. Thanks!

Copy link

codecov bot commented May 27, 2025

Codecov Report

Attention: Patch coverage is 98.70130% with 2 lines in your changes missing coverage. Please review.

Project coverage is 68.34%. Comparing base (b90c1ad) to head (9d61a04).
Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
onnxscript/ir/_convenience/_init_test.py 98.18% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2303      +/-   ##
==========================================
- Coverage   73.86%   68.34%   -5.53%     
==========================================
  Files         243      244       +1     
  Lines       31213    31363     +150     
  Branches     3537     3547      +10     
==========================================
- Hits        23055    21434    -1621     
- Misses       6930     8752    +1822     
+ Partials     1228     1177      -51     

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

"""Find the values that are considered as inputs and outputs in a sequence of nodes"""
# Search the unique inputs/outputs in new_nodes, keeping the order.
all_inputs = dict.fromkeys(sum([node.inputs for node in nodes], ()))
all_outputs = dict.fromkeys(sum([node.outputs for node in nodes], ()))

Check notice

Code scanning / lintrunner

RUFF/C419 Note

# Search the unique inputs/outputs in new_nodes, keeping the order.
all_inputs = dict.fromkeys(sum([node.inputs for node in nodes], ()))
all_outputs = dict.fromkeys(sum([node.outputs for node in nodes], ()))
# A value is considered as input if it is not any output.

Check notice

Code scanning / lintrunner

RUFF/C419 Note

@@ -0,0 +1,248 @@
# Copyright (c) Microsoft Corporation.

Check warning

Code scanning / lintrunner

RUFF/format Warning

Run lintrunner -a to apply this patch.
# Licensed under the MIT License.
"""Unit tests for the _constructors module."""

import onnx

Check warning

Code scanning / lintrunner

RUFF/I001 Warning

Import block is un-sorted or un-formatted.
See https://docs.astral.sh/ruff/rules/unsorted-imports

import onnx

import unittest

Check notice

Code scanning / lintrunner

PYLINT/C0411 Note

standard import "unittest" should be placed before third party import "onnx" (wrong-import-order)
See wrong-import-order. To disable, use # pylint: disable=wrong-import-order
@Johansmm
Copy link
Contributor Author

Johansmm commented Jun 3, 2025

@justinchuby I see that it is no longer possible to merge this work here. Do you want me to move it to ir-py ?

@justinchuby
Copy link
Collaborator

@justinchuby I see that it is no longer possible to merge this work here. Do you want me to move it to ir-py ?

Yes please, thanks!

@Johansmm
Copy link
Contributor Author

Johansmm commented Jun 3, 2025

Closing this PR, moving to onnx/ir-py#63

@Johansmm Johansmm closed this Jun 3, 2025
@github-project-automation github-project-automation bot moved this from Todo to Done in ONNX Script Review Board Jun 3, 2025
@Johansmm Johansmm deleted the introduce-node-convenience-functions branch June 4, 2025 07:53
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.

[IR] Implement graph editing APIs as convenience functions
2 participants