docs: document known limitations of _can_set_attn/experts_implementation source inspection#45338
Closed
RudrenduPaul wants to merge 2 commits intohuggingface:mainfrom
Closed
Conversation
The source-inspection heuristics in `_can_set_attn_implementation` and
`_can_set_experts_implementation` already contained a brief disclaimer
("ugly check based on opening the file"), but did not document the
specific failure modes. Expands both docstrings to cover:
- False positives when the searched strings appear in comments or
string literals rather than actual control-flow code
- Compiled modules (.so/.pyd) — already handled by the __file__ guard
- Non-standard import environments (PyInstaller, zipimport, custom
loaders) — also handled by the __file__ guard
- Interactive environments (Jupyter, REPL) — handled by __file__ guard
No behaviour change. Closes huggingface#45162.
Built by Rudrendu Paul, developed with Claude Code
Member
|
Hi, I'm sorry, we probably don't want this PR! The new docstring is very long, and most of these risks are obvious once you see that it's string-searching inside the file |
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
2 participants
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.
Closes #45162
What this PR does
Expands the docstrings of
_can_set_attn_implementationand_can_set_experts_implementationinmodeling_utils.pyto explicitly document the known limitations of their source-inspection heuristic.Coordination: I posted on the issue before opening this PR. The maintainer comment (#45162 (comment)) confirmed the approach is "definitely hacky" and that the team is "open to ideas" — this PR addresses the immediate documentation gap as a first step.
Limitations now documented
Both methods now document four known edge cases:
eager_attention_forward,ALL_ATTENTION_FUNCTIONS.get_interface(,@use_experts_implementation) may appear inside docstrings, comments, or string constants rather than actual code, causing a spuriousTrue..so/.pyd) — no readable source file. Already handled safely by the__file__guard (returnsFalse), but this was not documented.zipimport, custom loaders) —__file__may be absent or non-filesystem. Also caught by the guard, but undocumented.__file__. Caught by the guard.No behaviour change
This is a documentation-only PR. No logic was modified.
Testing
No tests changed.
make stylewas run to verify formatting.