Skip to content

Fix TRL optional dependency checks with Transformers 5 - #9770

Open
GodRayyy wants to merge 3 commits into
modelscope:mainfrom
GodRayyy:fix-trl-optional-dependency-check
Open

Fix TRL optional dependency checks with Transformers 5#9770
GodRayyy wants to merge 3 commits into
modelscope:mainfrom
GodRayyy:fix-trl-optional-dependency-check

Conversation

@GodRayyy

@GodRayyy GodRayyy commented Jul 20, 2026

Copy link
Copy Markdown

PR type

  • Bug Fix
  • New Feature
  • Document Updates
  • More Models or Datasets Support

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_ascend only. The same issue still affects weave and other TRL optional dependencies. This PR fixes the shared package check and keeps version queries unchanged.

Experiment results

  • Added tests for missing weave and vllm_ascend.
  • Added tests for missing TRL and newer TRL behavior.
  • Verified that return_version=True still returns the version tuple.
  • Verified that newer bool-return behavior is unchanged.
  • Verified GRPOTrainer imports with TRL 0.28.0 and Transformers 5.3.0.
  • Ran Qwen3.5-4B LoRA GRPO past step 500 with the same fix.

@gemini-code-assist gemini-code-assist Bot 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.

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.

Comment thread tests/utils/test_import_utils.py Outdated
Comment thread swift/utils/import_utils.py Outdated
@GodRayyy
GodRayyy deployed to qoder-review July 20, 2026 09:12 — with GitHub Actions Active

@qoderai qoderai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👋 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 QoderView workflow run

Comment thread swift/utils/import_utils.py
@GodRayyy
GodRayyy force-pushed the fix-trl-optional-dependency-check branch from 9ce8de8 to bc7baf1 Compare July 21, 2026 03:15
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.

1 participant