Conversation
Copilot
AI
changed the title
[WIP] custom_op executor AssertionError when running different tests together
Fix custom_op executor persisting in default executors after deregistration
Nov 3, 2025
crcrpar
reviewed
Nov 3, 2025
Collaborator
|
@Copilot could you change how we add Ref: lightning-thunder/thunder/torch/custom_op.py Lines 415 to 419 in f946af6 |
custom_op executor to the default executors to avoid hidden registration through thunder.torch.custom_op._register_custom_op
…ation When _deregister_custom_op is called, it now checks if there are no more custom ops registered and removes the custom_op_ex executor from the default executors list. This prevents the executor from persisting across test runs when no custom ops are actually registered. Co-authored-by: crcrpar <16191443+crcrpar@users.noreply.github.com>
Add test_custom_op_executor_cleanup to ensure that the custom_op executor is properly removed from default executors when all custom ops are deregistered. This prevents the bug from recurring where the executor would persist across test runs. Co-authored-by: crcrpar <16191443+crcrpar@users.noreply.github.com>
- Simplified custom_op_ex removal logic by removing unnecessary check - Removed placeholder issue URL from test docstring Co-authored-by: crcrpar <16191443+crcrpar@users.noreply.github.com>
Added detailed explanation in test docstring showing how the test directly addresses the original issue where test_recipes.py would fail after test_torch_library_custom_op.py due to custom_op remaining in get_expected_executors(). The test simulates the exact sequence that was failing and verifies the fix prevents the executor from persisting. Co-authored-by: crcrpar <16191443+crcrpar@users.noreply.github.com>
Signed-off-by: Masaki Kozuki <mkozuki@nvidia.com>
9c10f0a to
b0e9af1
Compare
Collaborator
|
it seems the Lit Job has error: is it related? |
Signed-off-by: Masaki Kozuki <mkozuki@nvidia.com>
kshitij12345
approved these changes
Nov 10, 2025
Collaborator
kshitij12345
left a comment
There was a problem hiding this comment.
LGTM, thanks @crcrpar
NOTE to reviewers: Also tested with
lightning-thunder/thunder/tests/test_extend.py
Lines 123 to 129 in bd2f0ed
t-vi
approved these changes
Nov 11, 2025
Collaborator
t-vi
left a comment
There was a problem hiding this comment.
Thank you @crcrpar @kiya00 @kshitij12345
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As per title. Currently
thunder.torch.custom_op._register_custom_opaddscustom_op_exto the default executor list.I chose this way because I found it not great to see an executor in the default even when that executor does nothing. But now I'd find it worse to have the hidden behavior than having an empty executor in the default list. Also this hidden automatic registration might've caused some test failures of #2710