Skip to content

Conversation

@viv-eth
Copy link
Contributor

@viv-eth viv-eth commented Jun 10, 2025

This patch introduces a node‐name mangling pass in the Deploy front end to ensure every node in the graph has a unique, non‐empty name. Previously, nodes generated by GraphSurgeon without an explicit name would collide, causing later layers to overwrite earlier layer bindings. With this change, each node is automatically assigned a name based on its operation and an incrementing counter, guaranteeing uniqueness and preserving all layer bindings.

Added

  • A new DeployTypes._mangleNodeNames(self) helper that:
    • Iterates over self.graph.nodes
    • Builds names of the form "{op_name}_{idx}", using the op’s class or string repr
    • Maintains a per‐op counter to ensure uniqueness
  • Invocation of self._mangleNodeNames() in frontEnd() immediately after tensor‐name mangling

Changed

  • Modified DeployTypes.frontEnd() to call _mangleNodeNames() before renaming graph inputs/outputs

Fixed

  • Unnamed GraphSurgeon nodes no longer default to "", so no two nodes share the same name
  • Prevents layer‐binding collisions that occurred when multiple unnamed nodes were emitted

PR Merge Checklist

  1. The PR is rebased on the latest devel commit and pointing to devel.
  2. Your PR reviewed and approved.
  3. All checks are passing.
  4. The CHANGELOG.md file has been updated.

@viv-eth viv-eth added the Bug Something isn't working label Jun 10, 2025
@Xeratec Xeratec added this to Deeploy Jun 10, 2025
@Xeratec Xeratec added this to the Release 0.2.0 milestone Jun 10, 2025
@Xeratec Xeratec moved this to In review in Deeploy Jun 10, 2025
@Xeratec
Copy link
Member

Xeratec commented Jun 10, 2025

Also, please do not forget to adjust the CHANGELOG ;) (This is also a note to me)

@Xeratec Xeratec moved this from In review to In progress in Deeploy Jun 16, 2025
@Xeratec
Copy link
Member

Xeratec commented Jun 19, 2025

@viv-eth Any plans to work on this in the next 2 weeks? Alternatively, I can offer to take it over, but will most likely include it in the next release.

Copy link
Member

@Xeratec Xeratec left a comment

Choose a reason for hiding this comment

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

Please rebase the PR and make sure to extend the CHANGELOG in the right place. Otherwise, the changes are good. Thanks a lot!

@Xeratec Xeratec self-assigned this Aug 11, 2025
@viv-eth viv-eth force-pushed the vivianep/mangleNodes branch from fffe9c7 to e0828c4 Compare August 16, 2025 23:29
@viv-eth
Copy link
Contributor Author

viv-eth commented Aug 16, 2025

e0828c4 should implement the requested changes @Xeratec .

@Xeratec Xeratec force-pushed the vivianep/mangleNodes branch from e0828c4 to 4e903b4 Compare August 19, 2025 15:14
Copy link
Member

@Xeratec Xeratec left a comment

Choose a reason for hiding this comment

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

The changes were added under v0.2.0 and not under v0.2.1. I did change it and merge once the tests succeed.

@Xeratec Xeratec moved this from In progress to Ready for Merge in Deeploy Aug 19, 2025
@Xeratec Xeratec merged commit f3da691 into pulp-platform:devel Aug 19, 2025
118 checks passed
@github-project-automation github-project-automation bot moved this from Ready for Merge to Done in Deeploy Aug 19, 2025
diaconuccalin pushed a commit to Aldrago98/Deeploy that referenced this pull request Oct 17, 2025
* [NetworkDeployer] Mangle these n...odes

* [DeeployTypes] Only mangle on duplicates

* Update changelog

---------

Co-authored-by: viv-eth <[email protected]>
diaconuccalin pushed a commit to diaconuccalin/Deeploy that referenced this pull request Oct 27, 2025
* [NetworkDeployer] Mangle these n...odes

* [DeeployTypes] Only mangle on duplicates

* Update changelog

---------

Co-authored-by: viv-eth <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants