Skip to content

Fix infinite recursion in graphdiff for non-torch graphs - #72

Merged
cymbalrush merged 1 commit into
apple:mainfrom
darkdi:graphdiff-recursion
Sep 2, 2026
Merged

cymbalrush merged 1 commit into
apple:mainfrom
darkdi:graphdiff-recursion

Conversation

@darkdi

@darkdi darkdi commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Both the Core AI implementation and the dispatcher below it are named _compute_op_diff_details, so the second one wins and its non-torch branch calls itself. Diffing a graph without torch_object ends in RecursionError. Renamed the first to sit beside _compute_torch_op_diff_details. Nothing imports tools/graphdiff and there are no tests for it, so say where you would want one.

@cymbalrush cymbalrush left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you!

@jakesabathia2

Copy link
Copy Markdown
Contributor

Thanks for the PR @darkdi!
Seems like you will need to sign the change before we could merge it :)

@darkdi

darkdi commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Right, that commit is unsigned. I need to get a signing key set up on this account first, then I will force push the same change signed.

@darkdi
darkdi force-pushed the graphdiff-recursion branch from afc0ec1 to 4b4ba2e Compare September 2, 2026 16:50
@darkdi

darkdi commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Signed and force pushed, the commit shows as verified now. The earlier merge commit is gone from the branch, it only pulled main in and main had not moved since.

@cymbalrush
cymbalrush merged commit f1bbf4f into apple:main Sep 2, 2026
2 checks passed
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.

3 participants