Skip to content
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

Implement a custom graph renaming routine for inline #84

Merged
merged 12 commits into from
Jun 12, 2023
Merged

Conversation

jbachurski
Copy link
Collaborator

Addresses #82 by avoiding adding extraneous Identity nodes when inlining and supporting subgraph renaming

  • Reimplements the onnx.compose.add_prefix_graph to make it more flexible.
  • Slightly expands Spox scopes to support 'anonymous' reserved names, which don't have an associated Var/Node, but are nevertheless marked as taken. This is because not all inlined nodes/values have a respective Var/Node.

Copy link
Collaborator

@cbourjau cbourjau left a comment

Choose a reason for hiding this comment

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

Great! Thanks! I just have some minor suggestions/questions.

This is because not all inlined nodes/values have a respective Var/Node.

Could you expand on this a bit? Do you mean the nodes inside a subgraph?

src/spox/_inline.py Show resolved Hide resolved
src/spox/_public.py Outdated Show resolved Hide resolved
@jbachurski
Copy link
Collaborator Author

Looks like due to the conda ONNX version bump, the CI is now failing on opset generation, so #83 will fix this :)

@jbachurski
Copy link
Collaborator Author

I also added an explicit proto-based test, and one for subgraph lists as there is no practical example that can be constructed there (no node uses a list of subgraphs...). This is since we are now straying from the ONNX utility which would have some tests itself. Maybe I'll upstream the new one at some point?

This is because not all inlined nodes/values have a respective Var/Node.

Could you expand on this a bit? Do you mean the nodes inside a subgraph?

@cbourjau Yeah - say an inlined ModelProto has an input X and output Y, and an intermediate Z. Then there is a Var corresponding to X and another for Y, but there will be no explicit variable for Z in the Spox graph. However, it does have to reserve some name for itself in-scope to avoid e.g. other inlines of the same model using that name. So the ScopeSpace object marks whatever name was used for Z as taken with the newly applied patch.

@jbachurski jbachurski merged commit 5fb9026 into main Jun 12, 2023
@jbachurski jbachurski deleted the improve-inline branch June 12, 2023 16:31
@cbourjau
Copy link
Collaborator

Thanks for the clarification :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants