Fix TRL optional dependency checks with Transformers 5 - #9770
Conversation
There was a problem hiding this comment.
Code Review
This pull request centralizes the patching of TRL package checks by introducing a new utility function patch_trl_package_check and adding corresponding unit tests. The review feedback highlights critical issues where importing trl.import_utils directly in both the utility and test files will raise a ModuleNotFoundError if the optional trl dependency is not installed. To resolve this, the reviewer recommends wrapping these imports in try-except blocks and skipping the tests when trl is unavailable.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
👋 Review Summary
Thanks for tackling the TRL 0.28 + Transformers 5 compatibility issue and consolidating the earlier vllm_ascend patch into a shared utility. The direction of centralising the logic in import_utils and adding focused tests is solid and makes the behavior much easier to reason about.
🛡️ Key Risks & Issues
- TRL helper monkey‑patching: patch_trl_package_check globally replaces the private helper trl.import_utils._is_package_available based on a heuristic sentinel check. This correctly fixes the tuple‑vs‑bool mismatch in the simulated scenario and for missing vllm_ascend/weave, but it also means any code in the process that relies on the original helper will now see our wrapped semantics. If TRL changes the helper’s signature or depends on tuple semantics in some paths, that could lead to subtle differences versus upstream behavior.
- Heuristic detection for buggy versions: The patch is triggered by checking whether _is_package_available('trl') returns a tuple when return_version=False. If some TRL/Transformers combinations only exhibit the bug for specific optional dependencies while keeping the core trl check as a bool, the patch would not activate and those optional dependencies would still be mis‑detected. This is a reasonable trade‑off for now, but worth documenting as an assumption and re‑evaluating if more version combinations appear in the wild.
🧪 Verification Advice
- In addition to the new unit tests, I’d recommend validating behavior across a small matrix of TRL/Transformers versions you care about (e.g., TRL 0.28.0, 0.28.x, and a newer release, combined with Transformers 5.x and 4.x) to ensure the sentinel-based patch does what you expect. In each case, confirm that is_vllm_ascend_available and is_weave_available behave as booleans and that return_version=True still returns a full (bool, version) tuple.
- It is also helpful to import grpo_trainer under mocked TRL conditions (tuple-returning helper vs. bool-returning helper) and run a minimal GRPO configuration to verify that backend selection and trainer initialization do not regress and that Qwen3.5-4B LoRA GRPO continues to run for a few hundred steps without unexpected warnings or errors.
💡 Thoughts & Suggestions
- Since this patch touches a private TRL helper and affects the whole process, consider briefly documenting the expected TRL and Transformers version ranges where the patch is intended to apply, plus the assumptions behind the tuple sentinel. That will help future maintainers reason about whether to keep, adjust, or remove this override when upgrading TRL.
- If you see more version-specific behavior from TRL’s optional dependency checks going forward, you might want to tighten the detection logic (for example by probing the optional packages you care about directly) or move towards a version-based guard rather than relying solely on the tuple-return sentinel.
🤖 Generated by Qoder • View workflow run
9ce8de8 to
bc7baf1
Compare
PR type
PR information
This fixes GRPO imports with TRL 0.28 and Transformers 5.
Transformers 5 returns a tuple from
_is_package_available. TRL 0.28 expects a bool. A missing optional package is therefore treated as installed.#8280 fixed this for
vllm_ascendonly. The same issue still affectsweaveand other TRL optional dependencies. This PR fixes the shared package check and keeps version queries unchanged.Experiment results
weaveandvllm_ascend.return_version=Truestill returns the version tuple.GRPOTrainerimports with TRL 0.28.0 and Transformers 5.3.0.