fix: unblock 4 tests deselected/skipped in #5312 (real bugs)#5359
Conversation
PR #5312 surfaced two real regressions by turning previously-silent skips into explicit `--deselect` / `pytest.skip(...)` blocks. Both were left as follow-ups rather than fixed in that PR. This PR fixes the underlying bugs so the suppressions can be dropped. 1. studio/backend/requirements/no-torch-runtime.txt: pin tokenizers Installing with `--no-deps -r no-torch-runtime.txt` (the path install.sh takes for the no-torch / GGUF-only mode) resolves transformers to 5.3.0 and tokenizers to the latest available (0.23.1). transformers 5.3.0 requires `tokenizers>=0.22.0,<=0.23.0`, so `from transformers import AutoConfig` then fails at import time: ImportError: tokenizers>=0.22.0,<=0.23.0 is required for a normal functioning of this module, but found tokenizers==0.23.1. Pin `tokenizers>=0.22.0,<=0.23.0` to match the constraint embedded inside every transformers version in the allowed window (4.56.0..5.3.0). Verified locally: a fresh `uv venv` + `uv pip install --no-deps -r no-torch-runtime.txt` followed by `from transformers import AutoConfig` now succeeds. Unblocks 3 deselected cases in studio-backend-ci.yml: - TestE2ETokenizersFix::test_autoconfig_works_with_no_torch_runtime (parametrized py 3.12 + 3.13 -> 2 cases) - TestE2EFullNoTorchSandbox::test_autoconfig_succeeds 2. unsloth/models/rl.py: defensive wrapper for _patch_trl_rl_trainers _patch_trl_rl_trainers has many internal `try: ... except: ... return` branches, but several paths (notably inspect.getsource on the thin wrappers TRL 1.x leaves in trl.trainer for trainers that moved to trl.experimental) can still propagate exceptions. The umbrella patch_trl_rl_trainers() ring-fences each call with try/except + warning_once, but direct callers (the CI shim in consolidated-tests-ci.yml, downstream tools, end-user scripts) used to see the raw exception, which forced #5312's CI heredoc to ring-fence with: except Exception as e: # TRL 1.x renames break the patch helper internally; we # accept that here and skip rather than fail the cell. pytest.skip(f"_patch_trl_rl_trainers raised: ...") Rename the existing implementation to _patch_trl_rl_trainers_impl and make _patch_trl_rl_trainers a thin wrapper that catches any uncaught exception and routes it through logger.info, matching the umbrella wrapper's behaviour. Power users who want the raw raising behaviour for their own diagnostics can still call _patch_trl_rl_trainers_impl directly. Adds tests/python/test_patch_trl_rl_trainers_defensive.py to lock the contract: the wrapper must never raise, and it must delegate to the impl on the happy path. Unblocks 1 skip in consolidated-tests-ci.yml's test_compile_sft_trainer_patch. Follow-up for #5312 once this lands: drop the two `--deselect` lines in studio-backend-ci.yml's repo-cpu-tests step and drop the `except Exception ... pytest.skip(f"_patch_trl_rl_trainers raised: ")` block in consolidated-tests-ci.yml's test_compile_sft_trainer_patch.
danielhanchen
requested review from
Datta0,
pluesclues and
rolandtannous
as code owners
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e774494427
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # `from transformers import AutoConfig` then fails at import time: | ||
| # ImportError: tokenizers>=0.22.0,<=0.23.0 is required ..., | ||
| # but found tokenizers==0.23.1. | ||
| tokenizers>=0.22.0,<=0.23.0 |
There was a problem hiding this comment.
In the studio-backend-ci.yml Shell installer tests step I checked, tests/sh/test_torch_constraint.sh is run on every backend CI job, and it still asserts grep -c '^tokenizers$' and derives the tokenizers line number from that exact match. With this new pinned requirement, bash tests/sh/test_torch_constraint.sh now fails the “tokenizers present as standalone line” and “tokenizers before transformers” checks, so this change blocks that CI job until the structural test accepts the versioned tokenizers spec.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request pins the tokenizers dependency to ensure compatibility with transformers during dependency-free installations and introduces a defensive wrapper around _patch_trl_rl_trainers to prevent exceptions from propagating to direct callers. New regression tests are added to verify this error-handling behavior. Feedback suggests using logger.warning_once instead of logger.info when patching fails to maintain consistency with established patterns in the codebase and ensure visibility of issues.
| logger.info( | ||
| f"Unsloth: Could not patch trl.trainer.{trainer_file}: " | ||
| f"{type(e).__name__}: {e}" | ||
| ) |
There was a problem hiding this comment.
The wrapper uses logger.info to log patching failures, but the "umbrella" function patch_trl_rl_trainers (line 1973) uses logger.warning_once. To maintain consistency with the intended "umbrella behavior" mentioned in the PR description and to ensure that unexpected patching failures are sufficiently visible without flooding logs, consider using logger.warning_once here as well. This also ensures the specific error details are captured dynamically.
| logger.info( | |
| f"Unsloth: Could not patch trl.trainer.{trainer_file}: " | |
| f"{type(e).__name__}: {e}" | |
| ) | |
| logger.warning_once( | |
| f"Unsloth: Could not patch trl.trainer.{trainer_file}: " | |
| f"{type(e).__name__}: {e}" | |
| ) |
References
- Prioritize consistency with established coding patterns within a module.
- User-facing warning messages should be dynamically generated to include the specific configuration values they refer to, rather than using hardcoded examples.
Drop verbose justifications down to one or two lines per site. The PR description carries the full context; in-file comments only need to point at the WHY.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8386322315
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Must match the bound every transformers 4.56..5.3 declares. | ||
| # With --no-deps, unpinned resolves to 0.23.1+ and `from transformers | ||
| # import AutoConfig` raises ImportError at import time. | ||
| tokenizers>=0.22.0,<=0.23.0 |
There was a problem hiding this comment.
When the e2e tokenizer suite is enabled, TestE2ETokenizersFix::test_negative_control_no_tokenizers still creates its "without tokenizers" requirements copy by dropping only lines whose stripped text equals tokenizers; this new versioned requirement survives that filter, so the copied file still installs tokenizers and AutoConfig succeeds, causing the negative-control assertion to fail. Adjust the test/filter alongside this requirement change so the no-tokenizers copy actually removes tokenizers>=....
Useful? React with 👍 / 👎.
tokenizers 0.23.0 was never published to PyPI (versions go 0.22.2 -> 0.23.1), so `tokenizers<=0.23.0` resolves to 0.22.2 in practice, the same version the explicit >=0.22.0,<=0.23.0 pin resolved to. Verified on Python 3.12 and 3.13.
danielhanchen
deleted the
fix/no-torch-tokenizers-and-trl1-patch-defensive
branch
Address bot review feedback on #5361: * Codex (P2): the previous relaxed regex `^tokenizers([<>=!,~ ]|$)` accepted any version operator, so `tokenizers>=0.23.1` would still pass the test even though that line would re-introduce the import failure #5359 fixed. * Gemini (medium): the boundary char class did not cover all PEP 508 separators (`[`, `;`, `@`). Replace the single check with two: 1. Loose: `^tokenizers([^a-zA-Z0-9._-]|$)` confirms the package is listed (covers extras, env markers, URLs, bare line). 2. Tight regression guard: pipe those lines through a second grep that requires `<=0.23.0` or the functionally equivalent `<0.23.1`. Rejects bare `tokenizers`, `>=0.22.0` (no upper bound), `>=0.23.1`, `!=0.23.0`, `<=0.24.0`, etc. Verified locally: - Current main (tokenizers<=0.23.0): 25 PASS, 0 FAIL. - Spot-check with the bug reverted (bare `tokenizers`): the new "tokenizers pinned with upper bound excluding 0.23.1+" check FAILS as intended; the original "listed" check still passes.
* fix(tests/sh): accept pinned tokenizers line after #5359 #5359 pinned the tokenizers line in studio/backend/requirements/no-torch-runtime.txt from bare `tokenizers` to `tokenizers<=0.23.0` to stop pip from resolving to 0.23.1+ (which transformers rejects at import time). The shell test in tests/sh/test_torch_constraint.sh was still asserting the literal `^tokenizers$` regex, which fails on the pinned form. Surfaced as a hard fail on PR #5312's Backend CI Repo tests (CPU) step: === Structural: tokenizers in no-torch-runtime.txt === FAIL: tokenizers present as standalone line (expected '1', got '0') FAIL: tokenizers before transformers (expected 'yes', got 'no') Relax the regex to `^tokenizers([<>=!,~ ]|$)` so it matches both bare and version-constrained forms, which preserves the original intent (verify tokenizers is present in the file, before transformers). Verified locally: 24 PASS, 0 FAIL. * fixup(tests/sh): tighten tokenizers check to guard the safe bound Address bot review feedback on #5361: * Codex (P2): the previous relaxed regex `^tokenizers([<>=!,~ ]|$)` accepted any version operator, so `tokenizers>=0.23.1` would still pass the test even though that line would re-introduce the import failure #5359 fixed. * Gemini (medium): the boundary char class did not cover all PEP 508 separators (`[`, `;`, `@`). Replace the single check with two: 1. Loose: `^tokenizers([^a-zA-Z0-9._-]|$)` confirms the package is listed (covers extras, env markers, URLs, bare line). 2. Tight regression guard: pipe those lines through a second grep that requires `<=0.23.0` or the functionally equivalent `<0.23.1`. Rejects bare `tokenizers`, `>=0.22.0` (no upper bound), `>=0.23.1`, `!=0.23.0`, `<=0.24.0`, etc. Verified locally: - Current main (tokenizers<=0.23.0): 25 PASS, 0 FAIL. - Spot-check with the bug reverted (bare `tokenizers`): the new "tokenizers pinned with upper bound excluding 0.23.1+" check FAILS as intended; the original "listed" check still passes.
1 participant
Follow-up to #5312. That PR surfaced two real regressions by turning previously-silent skips into explicit
--deselect/pytest.skip(...)blocks, and explicitly left both as follow-ups. This PR fixes the underlying bugs so the suppressions can be dropped.1.
tokenizersunpinned inno-torch-runtime.txtbreaks transformers importInstalling with
--no-deps -r studio/backend/requirements/no-torch-runtime.txt(the pathinstall.shtakes for the no-torch / GGUF-only mode) resolves transformers to 5.3.0 and tokenizers to the latest available (0.23.1). transformers 5.3.0 requirestokenizers>=0.22.0,<=0.23.0, sofrom transformers import AutoConfigthen fails at import time:Pin
tokenizers>=0.22.0,<=0.23.0to match the constraint embedded inside every transformers version in the allowed window (4.56.0..5.3.0).Verified locally with the exact install path: fresh
uv venv->uv pip install --no-deps -r no-torch-runtime.txt->from transformers import AutoConfignow succeeds.Unblocks 3 deselected cases in
studio-backend-ci.yml:TestE2ETokenizersFix::test_autoconfig_works_with_no_torch_runtime(parametrized py 3.12 + 3.13 -> 2 cases)TestE2EFullNoTorchSandbox::test_autoconfig_succeeds2.
_patch_trl_rl_trainersraises on TRL 1.x_patch_trl_rl_trainershas many internaltry: ... except: ... returnbranches, but several paths (notablyinspect.getsourceon the thin wrappers TRL 1.x leaves intrl.trainerfor trainers that moved totrl.experimental) can still propagate exceptions. The umbrellapatch_trl_rl_trainers()already ring-fences each call withtry/except + warning_once, but direct callers (the CI shim inconsolidated-tests-ci.yml, downstream tools, end-user scripts) used to see the raw exception, which forced #5312's heredoc to ring-fence with:Rename the existing implementation to
_patch_trl_rl_trainers_impland make_patch_trl_rl_trainersa thin wrapper that catches any uncaught exception and routes it throughlogger.info, matching the umbrella wrapper's behaviour. Power users who want the raw raising behaviour for their own diagnostics can still call_patch_trl_rl_trainers_impldirectly.Adds
tests/python/test_patch_trl_rl_trainers_defensive.pyto lock the contract: the wrapper must never raise, and it must delegate to the impl on the happy path. 5 cases, all pass locally on TRL 1.4.0.Unblocks 1 skip in
consolidated-tests-ci.yml'stest_compile_sft_trainer_patch.Follow-up for #5312
Once this lands, #5312 should rebase and drop:
--deselectlines instudio-backend-ci.yml'srepo-cpu-testsstepexcept Exception ... pytest.skip(f"_patch_trl_rl_trainers raised: ")block inconsolidated-tests-ci.yml'stest_compile_sft_trainer_patchTest plan
from transformers import AutoConfigsucceeds in a fresh--no-deps -r no-torch-runtime.txtvenv (Python 3.12).tests/python/test_patch_trl_rl_trainers_defensive.pypasses (5/5) on TRL 1.4.0._patch_trl_rl_trainers("sft_trainer")still patches via the impl on TRL 0.x and TRL 1.x (verified in local sandboxes).