Skip to content

Implement all_nodes() and subgraphs() as convenience methods #76

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

justinchuby
Copy link
Member

Suggested by @xiaoyu-work, this PR created convenience methods all_nodes() and subgraphs() for easier access to the nested nodes and subgraphs. They are created so that it is easier to discover the methods.

@justinchuby justinchuby requested review from titaiwangms and a team as code owners June 13, 2025 20:24
@justinchuby justinchuby requested a review from Copilot June 13, 2025 20:24
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements two convenience methods, all_nodes() and subgraphs(), to simplify traversals of nodes and subgraphs in both Graph and Function objects.

  • Added all_nodes() and subgraphs() to the Graph class for nested node access.
  • Modified graphs() in the model to yield the main graph and its subgraphs.
  • Added analogous all_nodes() and subgraphs() methods to the Function class leveraging its underlying _graph.
Comments suppressed due to low confidence (3)

src/onnx_ir/_core.py:2385

  • The subgraphs() method in Graph compares node.graph with self, while in Function it compares with self._graph. Consider adding a comment to clarify this intentional difference to aid API consistency.
if graph is self:

src/onnx_ir/_core.py:2905

  • [nitpick] Since the graphs() method now yields self.graph before yielding subgraphs, consider updating the docstring to explain the intentional order, so that consumers of this API are clear on its behavior.
yield self.graph

src/onnx_ir/_core.py:3056

  • In the Function class, subgraphs() checks if node.graph equals self._graph instead of self. Confirm and document this difference in comparison to the Graph implementation to ensure clarity.
if graph is self._graph:

Copy link

codecov bot commented Jun 13, 2025

Codecov Report

Attention: Patch coverage is 54.16667% with 11 lines in your changes missing coverage. Please review.

Project coverage is 73.35%. Comparing base (24096c6) to head (4e02e5e).

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/onnx_ir/_core.py 54.16% 10 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #76      +/-   ##
==========================================
- Coverage   73.48%   73.35%   -0.14%     
==========================================
  Files          37       37              
  Lines        4507     4526      +19     
  Branches      906      910       +4     
==========================================
+ Hits         3312     3320       +8     
- Misses        865      875      +10     
- Partials      330      331       +1     

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

Signed-off-by: Justin Chu <[email protected]>
@justinchuby justinchuby added the do not merge Do not merge yet label Jun 13, 2025
@justinchuby
Copy link
Member Author

@codecov-ai-reviewer test

Copy link

codecov-ai bot commented Jun 14, 2025

On it! Codecov is generating unit tests for this PR.

@justinchuby
Copy link
Member Author

@codecov-ai-reviewer test

Copy link

codecov-ai bot commented Jun 14, 2025

On it! Codecov is generating unit tests for this PR.

Copy link

codecov-ai bot commented Jun 14, 2025

Sentry has determined that unit tests are not necessary for this PR.

@justinchuby justinchuby added this to the 0.1.2 milestone Jun 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants