fix: handle symlinked Transformers module cache - #3914
Conversation
Signed-off-by: larkzhang-nv <larkz@nvidia.com>
Signed-off-by: larkzhang-nv <larkz@nvidia.com>
Signed-off-by: larkzhang-nv <larkz@nvidia.com>
829b334 to
881df04
Compare
|
/ok to test 881df04 |
jQizhang
left a comment
There was a problem hiding this comment.
Reviewed by a team of specialized agents (RL expert, bug finder, test agent, design reviewer, devil's advocate).
Verdict: correct, well-scoped, and faithful to upstream. Nothing blocking. Independently verified:
- The backport body is byte-for-byte identical to upstream v5.13.0
_compute_local_source_files_hash— no divergence in the resolve trick, sort key, encoding, or hash truncation. - The
[5.11.0, 5.13.0)gate is right at both bounds: v5.10.0 and earlier use a different 4-argmodules_neededimplementation that derives sibling paths from the snapshot dir and is not affected; the crash-shaped 2-argget_relative_import_filesform first appears at v5.11.0. - The bug reproduces (
FileNotFoundError: .../blobs/dependency.py), andtest_get_cached_module_file_handles_symlinked_hub_cachegenuinely fails without the fix — confirmed by restoring the stock 5.12.1 function. That's a real regression test, not a tautology. - The monkeypatch reaches the sole call site (module-global lookup at
dynamic_module_utils.py:444); nofrom ... importalias anywhere in transformers bypasses it. - Hashes are unchanged for ordinary non-symlinked model dirs, so this does not re-key existing dynamic-module caches.
- The new test file is collected by
L0_Unit_Tests_Other.shand is not excluded from Lfast;pyrefly.tomlis correctly updated;ruff check,ruff format --check, andpyrefly checkare clean on all changed files.
Thanks for the narrow version gate, the fail-loud signature guard, and especially the offline e2e GRPO validation on nemotron-omni-30b — that's more evidence than this class of fix usually gets.
The three inline comments are all polish; none of them affect correctness. Several other candidate findings were raised and deliberately cut as out of proportion, including pre-release version-gate edges (unreachable under the current pins) and a skipif for the eventual 5.13 bump — on that last one we concluded the current hard failure is the better behavior, since it forces deletion of this module rather than letting it rot silently.
Generated by Claude Code
terrykong
left a comment
There was a problem hiding this comment.
Reviewed by a team of five agents (RL expert, bug finder, test author, design reviewer, devil's advocate) at 829b334, re-anchored to the current head after your rebase.
This is a careful PR and I'd approve it. Things I verified by running them rather than by reading:
- The backport is byte-for-byte upstream v5.13.0 — same hash inputs, sort key and truncation. No drift.
- The regression test is genuine: restoring the pristine 5.12.1 implementation makes it fail with
FileNotFoundError: .../blobs/dependency.py, the same shape you describe. - No cache churn. For a plain model directory the patched and unpatched digests are identical (
67b90f247d23dbcd), so existingHF_MODULES_CACHEentries are not invalidated — and that digest also matches what 5.13.0 produces, so there is no cache split when the pin eventually moves. - The version gate is right at both ends. 5.10 genuinely predates the bug (5.11.0 is where
get_relative_import_filesstarted being called on an already-resolved path), and thetrtllmextra'stransformers==5.5.4correctly no-ops instead of tripping the signature check. - The new test file is collected by
L0_Unit_Tests_Other.sh, at an affected Transformers version.
The signature check, the idempotence check, and citing the upstream PR in both the docstring and the log line are all more care than the neighbouring patches in nemo_rl/__init__.py take. Worth keeping.
On your self-review: you'd already found the sys.path leak in the symlink test and the over-claiming "Keep Transformers optional" comments, so I've dropped my versions of both — yours are the better write-ups, and your framing of the comment issue ("under the shipped pins it never does") is more precise than mine was. That leaves the two below, neither of which overlaps with your three.
Two notes on the description, no action needed: it says "27 focused unit tests passed" where the file has 13, and that 5.13.0 "successfully used the upstream dynamic-module cache implementation" — the shipped test file does fail under 5.13.0, which is what the second comment is about.
Generated by Claude Code
Signed-off-by: larkzhang-nv <larkz@nvidia.com>
|
/ok to test f654713 |
Signed-off-by: larkzhang-nv <larkz@nvidia.com>
|
/ok to test b2be308 |
Signed-off-by: larkzhang-nv <larkz@nvidia.com>
|
/ok to test 7709c54 |
|
auto closed through #3906 with incorrect settings, reopened. |
Signed-off-by: larkzhang-nv <larkz@nvidia.com>
|
/ok to test ec8dc0d |
|
Hi, @yuki-97 @terrykong Do you think we should merge this PR soon? It would unblock nightly test for vlm. |
|
Hi @ZhiyuLi-Nvidia , I’ve resolved both threads since neither affects correctness. Could you please check if it can be merged? |
Thank you @jQizhang a lot. |
What does this PR do ?
Backports the Transformers dynamic-module symlink-cache fix while NeMo RL remains on Transformers versions earlier than 5.13.0.
With
trust_remote_codemodels in offline mode, affected Transformers 5.11 and 5.12 releases can resolve Hugging Face snapshot symlinks intoblobs/before discovering sibling relative imports. This produces invalid paths such asblobs/configuration_nemotron_h.pyand breaks model configuration loading.This PR installs a narrowly version-gated compatibility patch for
transformers>=5.11,<5.13, validates the expected upstream function signature, and exercises the public dynamic-module cache path with a symlinked snapshot and transitive relative imports.The implementation is adapted from huggingface/transformers#46618, which shipped in Transformers 5.13.0. The compatibility module and its
nemo_rlbootstrap can be removed once NeMo RL requires Transformers 5.13.0 or newer.Issues
N/A
Usage
No user action is required; the compatibility patch is applied during
nemo_rlpackage initialization only for affected Transformers versions.Testing
End-to-end offline GRPO validation
Ran
tests/test_suites/vlm/vlm_grpo-nemotron-omni-30ba3b-clevr-1n8g-megatron-tp8ep8.v1.shwith Transformers 5.12.1 in Hugging Face offline mode. With this fix applied, the model loaded successfully and the 10-step GRPO test completed successfully.Before your PR is "Ready for review"
Pre checks:
Additional Information