4 commits

Author	SHA1	Message	Date
Daniel Han	50b9deacb0	Trim comments in tests (#13844) ... Comment and docstring changes only, in test files, split out of #12968 so the test-only part can merge first.	2026-10-15 05:30:27 -07:00
Daniel Han	971eb36f48	Trim comments in tests, scripts, CI helpers and unsloth_cli (#10113) ... * Trim comments in tests, scripts and CI helpers Comment-only pass over 453 changed files under tests/ (excluding the studiobench harness), scripts/ and .github/scripts/. 17,749 -> 10,635 comment lines (-40.1%) 1,183,079 -> 757,190 comment characters (-36.0%) Distinctive-fact retention against the base commit: 93.2%. A sample of 100 restoration and deletion decisions was judged by hand: 98 correct. No comment line exceeds 120 physical characters. Nothing outside comments changes: no code, docstrings, string literals, licence headers or pragmas. Verified with an AST code-only signature of every file against the base. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Put the moved comments back on the code they describe The signal-handler rationale in launch.py had been reattached to the handler registration loop, which has no try/finally, and its except-branch note had been split into a fragment starting mid-sentence with the noqa marker duplicated into the prose. The PORT_FILE launcher comments had been reattached to the _studio_root_id caching test, which does not touch PORT_FILE. * Put the section banners back, and use plain dashes in comments The trim removed the rule-line comments that index these files. Tests locate a region by finding its banner and end it on the next one, so a missing banner is a broken anchor rather than lost decoration. Each is restored above the code line it preceded at 1d2679f; where that anchor is not unique the banner is left out rather than guessed at. Em and en dashes are rewritten as plain dashes inside comment tokens only. String literals are code and are untouched, so the diff stays comment-only. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Put reattached comments back on the code they describe The comment pass moved a number of explanatory comments off the code they were written for and onto unrelated statements nearby, so they now read as false or as truncated fragments. This restores each one at its correct site: - lint_duplicate_definitions: the import-key rationale goes back on the emitted Finding key instead of being half-pasted onto `implicit =`. - lockfile_supply_chain_audit: "Empty by default" belongs on the empty CARGO_IOC_STRINGS tuple, not on the non-empty source allowlist; the missing-lockfile and traceback notes go back on their own branches. - scan_packages: "archives landed in dest" describes the successful pip download, not `if not downloaded:`; the backslash-continuation note goes back on the continuation check in _scan_line_end. - interrupted_install_probe: the start_new_session/getpgid note belongs on the posix branch that calls getpgid, not on the win32 branch. - virustotal_scan: the missing-secret rationale goes back on the skip path and the :⚠️: note back on the detection print. - scan_npm_packages: the rmtree cleanup note goes back on the rmtree. - check_frontend_dep_removal: one per-key rationale per key again, instead of five concatenated onto browserslist. - profile_startup: failed-launch and nothing-measured notes swapped back. - enforce_kwargs_spacing: fold-rejection reasons back on the guards that reject for them, and the f-string fold comment back in order. - tests/conftest: the bitsandbytes broken-wheel note back on the bitsandbytes handler. Comments only; no code, string literal, docstring or pragma changes. * Repair the banner restore: drop duplicate headings, keep the blank line The scripted restore re-added each section banner without removing the de-bannered plain heading the trim had left behind, so a heading appeared twice, once ruled and once bare. It also inserted the banner flush against the next line of code where the base had a blank line after it, which turns a section separator into a doc comment for whichever def follows. Both are the same fault the trim itself was being screened for, introduced by the remediation rather than by the trim. * Reattach 12 review comments to the code they describe Each of these was a rationale that ended up on a neighbouring test or assertion, or a fragment cut mid-sentence, so it described something other than the line under it. - test_gguf_disk_preflight: the 3GB-embeddings arithmetic belongs to the untied case, not the tied one (2GB there). - lockfile_supply_chain_audit: reassemble the workflow-command escaping rationale on the skip branch. - studio_smoke/multi_turn_chat: restore the retry-policy explanation in main(). - test_virustotal_scan: restore the advisory-posture rationale on the scan step. - test_amd_fastpath_probe: return each ROCm rationale to its parameter row. - test_cuda_repair: the Blackwell span note goes back on the Blackwell test; complete the Kepler sentence. - test_windows_rocm_bnb_version: realign the sitecustomize / opt-out rationales with their cases. - test_dataset_num_proc: the Pool(1) note belongs to the low-memory auto path. - test_install_whisper_prebuilt_logic: decoy-DLL, libomp, build-bump and same-ggml notes back on their own tests; the link copy fallback needs none. - playwright_loaded_models_indicator: stale-eject notes back on their sections and the preference banner restored. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Restore the review comments that were cut mid-sentence or reattached to the wrong code * Restore the comment tails the 120-column pass cut off The trim enforced the column limit by chopping the physical line rather than rewrapping it, so a block's last sentence stopped mid-thought and the clause carrying the reason was gone: -# ...fails in a second rather than +# ...fails in a second rather than after the quantise and the multi-gigabyte save. -# fp8 / mxfp8 need bf16 weights, so skip non-bf16 Linears; +# fp8 / mxfp8 need bf16 weights, so skip non-bf16 Linears; nvfp4 handles fp32. Mirrors the runtime gate. 883 blocks across 266 files, restored from the merge base and rewrapped at word boundaries within 120 columns. Blocks that end on proper punctuation followed by a new sentence are left alone: those were shortened on purpose, and only a base that continues the same sentence where the head stops counts as a cut. No word is invented; every restored word comes from the base. Pragmas, shebangs, coding lines, licence text and rule-line banners are skipped. Dangling truncations drop from 336 to 77. Full suite is 65 failed / 18683 passed, matching main exactly, so no test changes behaviour. * Restore comment tails cut inside a parenthetical or abbreviation The first repair pass keyed on terminal punctuation, so a block cut after "(e.g." or mid-code-span looked finished and was left alone. Balance the brackets and backticks instead, and treat a trailing abbreviation as mid-sentence: -# ...skip the whole module up front on runners without it (e.g. +# ...skip the whole module up front on runners without it (e.g. CPU-only) before importing unsloth, otherwise +# collection errors instead of producing a skip. 22 further blocks across 20 files. _ts_deps.py is fixed by hand: it had also lost a sentence from the MIDDLE of the block, so it was not a prefix of the base text and no prefix rule could reach it. * Reassemble the review comments that were left scrambled or cut short Restore the full rationale for the comments the trim pass left as reversed or truncated fragments, and put the openenv, reasoning-effort and recovery window notes back on the code they describe. * Put the scrambled comment blocks back over the code they describe * Put the inline labels back on the entries they label * Restore an inline label lost in the merge * Restore the facts the reworded comments dropped * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Move the displaced notes back onto the tests they describe * Re-pair the fixture rationales with their fixtures * Put the relocated notes back and finish the sentences the trim cut * Restore the dropped facts and correct the ones that were never true * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Unfuse the merged notes and drop the reformatting the trim smuggled in * Restore the clauses the trim cut off and unfuse the notes it merged Thirty sites where a comment was shortened past the point it still said anything. Most ended on a colon or a dangling word, so the reason the branch exists was gone while the sentence introducing it stayed. The rest had absorbed a neighbouring test's note, which left one row documented as another row's behaviour. Each restored clause was checked against the code under it rather than copied back on trust. Three were not restored: a pointer to a measurement that is not in the function it names, a claim that a missing import fails as NameError when it fails at collection, and a redundant restatement in the autoscroll check. tests/kaggle/t4_smoke/training_evidence.py ends up identical to main, which is the right outcome for a file whose only trimmed sentence was load-bearing. * Give back the halves of the comments the trim cut off Two hundred and twenty comments across a hundred and twenty files had been shortened to the sentence that introduces a fact and none of the fact. A note ending on a colon or on "so" or "carried over" still reads as a comment and still parses, which is why every structural check has been passing over them: a truncated comment is a well-formed comment. Most of these were only findable by asking what a block ENDS on rather than what it starts with. The seven passes before this one all keyed on the first line, so they were blind to the commonest damage in the whole trim. The supply-chain audit had seven "Mini Shai-Hulud May-12 wave:" headers with the package names deleted, which was the entire content of each one. A handful were worse than truncated. A "can't fit on one line, so leave it" note had moved from the test asserting changed is False onto the one asserting changed is True; the sm_61 and sm_70 rationales in the CUDA repair tests were stacked on a test that exercises neither, leaving their own tests bare; an unpinned-latest note sat on a test that stubs a working install on disk. Restored clauses were checked against the code under them, not copied back on trust. Around a dozen were left out or rewritten because the base was wrong about the code as it stands: a pip stub credited with an append it does not do, five exports said to be filled by a loop that fills one, a __dict__ rationale that argues for the opposite of what the loop needs, and UI strings quoted as current that the sources no longer use. * Put the fused row notes back on the rows they are true of Ten sites where a comment described a sibling. The torchao skew table had three rows' notes concatenated onto the 0.18.0+cu130 row; the revision-forwarding table had the adapter-tokenizer exception one row below the only row it holds for; the vision LoRA pair had the tied and untied cases on a single anchor. Four of these were mine, not the trim's. The 120-column rewrap left an orphaned continuation line sitting on a class, one repair copied an untagged-build note onto a fixture that pins a URL and has no untagged build in it, another duplicated a namespace-package note and pushed the interrupted-install label onto the wrong row, and a fourth moved a collision note onto a fixture with a single host, where it is false. A pass that rewrites 220 comments will get some of them wrong, and these are the ones it got wrong. Two reported sites were left alone. The masking note in the VirusTotal client already carries its full clause, reflowed; the IOC scan comment reads complete, and its missing "4." went with the numbering on every other step in that function. * Move the row and branch notes back onto what they describe Nineteen sites. The token-count parity table had every row comment one param too low, so each seeded reasoning style was documented as the next one; the font-scale contract had two rationales slid down a continue each; the validator fixture banner for R-INST-001 sat under R-INST-005, leaving three tests unlabelled and two double-labelled. Three of these I caused. The parity table was misplaced by the previous commit, which moved fused notes onto rows without checking the seeds underneath them; a timing bound landed in a test that has no timing assertion; a restore note was reinserted between the two commands it claims to cover instead of above them. The pacing benchmark had lost its measurement table entirely, including the #7892 result that is the reason there are two budgets rather than one headline number. Restored with the columns intact. * Undo the damage the last four repair passes did Twenty-five sites, and almost none of them were the trim's. Round nine was called "move the row and branch notes back onto what they describe", and it moved a good many onto things they do not describe: the xpu swap tests had the pinned-mirror rationale on the test that asserts the index is dropped, the cuda repair file had a whole run of rationales slid one test upward, and several notes were duplicated onto a neighbour rather than moved. Where a clause was already correct on its real anchor, the copy is deleted rather than parked somewhere new. Separately, this PR had been changing code, and the gate could not see it. Five files: three where a repair split a tuple across lines to make room for a restored trailing comment, two where the trim deleted a trailing comment and then joined up the wrapping that existed only to carry it. Parentheses and line breaks leave the parse tree untouched, so an AST comparison calls all of it comments. Every one is back to base byte for byte, with the comments that justified the wrapping restored. scripts/code_reflow.py is the check for it: same tokens in a different arrangement is a code change, whatever the AST says. * Let ruff rejoin the two lines the shorter comments freed Both were split only to make room for a trailing comment. Round ten cut those comments back to what is true of the row, the expressions now fit, and the formatter joins them. Running it is not optional: pre-commit.ci was red on exactly these two. * Delete the stray copies rather than move them again Five sites. Three were duplicates: the correct sentence still sits on the test it describes, and a repair pass left a second copy on a neighbour. Moving those would have been the fourth round of shuffling one clause between two tests, so the copies are simply gone. The mislabelling was the point in each case. An empty handover tag and an absent one are different precedence rules, and the PowerShell 7.5 parsing note had been copied from the empty-string test onto the one whose variable is unset. A non-zero-exit rationale sat on a parser test that asserts a successful exit. The other two are the trim's own: "#5854)," with its subject cut away, and a regex description that stopped before naming the self. prefix it matches, which is what explains the narrower filter below it. * Put the nine slid notes back on what they assert Seven had moved one branch or one call down. The autocast one had landed on the case that forces float32 and asserts dtype float16, directly under a note saying there is no dtype at all; the JAX rationale sat on the USE_FLAX env-var case, whose module map has no jax in it; the kill-and-sentinel note sat on a test that only checks a GPU row regex. Each is back on the assert it describes, with a tenth found in the same file as the probe timeouts. Two are the trim's own. A mid-block deletion in the settings-tabs driver spliced two half sentences into "blocking a / error-handling regression and was a stale constant"; the macOS slot table lost the only statement of why macos-14 is absent from a set named for images GitHub still schedules. The retirement dates are restored, verified against all three workflows that carry them. The settings-tabs note is shorter than base on purpose. It now says the invariant, that blocking a panel must not collapse the dialog, and drops the tab count, which nothing asserts. * Move twelve more notes back onto the branch they are about All twelve had slid onto an adjacent test or branch, and in each the displaced note contradicted what it now sat on. The CPU-backend note about a wheel being left alone had landed on the test asserting a repair runs; the XPU one about a dispatched shard sat on the single-device case; the redirect-body note sat in a test that never binds served; the draft-state refusal sat on the test where the target is present and the run returns 0. One lost text on the way as well: the missing-target rationale had been cut to its first two sentences, dropping the reason the draft and prerelease refusal above it would otherwise degrade into no check. Also a rewrap that duplicated its own tail instead of replacing it, so the merge-budget note read "else a sharded own device, not GPU0, else a sharded model OOMs". * Return six notes to the test whose behaviour they describe Eight reports, six defects: two pairs described the same misplacement from either side. One was worse than misplaced. "A comment naming a probe is not a probe" had moved onto the metadata scan, whose predicate matches the raw line with no comment handling at all, so it claimed an exemption that scan does not implement. It belongs on the Test-Path scan, the only one that splits on # before matching. Moved, not reworded. The rest argued against the test they sat on. The lazy-trl note was on a stub with no __getattr__ to raise; the bogus-variant note was above a request sending a valid variant; the imported-TensorFlow note was on the case that supplies no tensorflow and asserts the flag is cleared; the 62-token rationale was on the test that asserts 12. * Move ten more notes onto the statement they explain Seven had slid off their anchor, and in most the stray was also cut short, so the fix was to restore the whole sentence where it is true and delete the fragment. The torchao dependency reason sat on a test about re-verification rather than on the --no-deps assertion it justifies; the bitsandbytes heading sat on a test that fetches quantizers_utils.py; the no-selection latch note sat on the delete block, which asserts nothing reaches the action. Two are the trim's own truncations. The stand-aside rationale in the shortcut driver stopped at a colon, losing why Enter on a focused Deny button would otherwise be defaulted away into approve. The kwargs-union case stopped at a semicolon, and the trim had eaten the leading ** from **kwargs as well. One was about the driver rather than a test: the JSON note sat beside the final print, where it described the document, instead of on the stderr redirection that is what actually keeps stdout clean. * Restore what the reflow cut, including three markers it mangled Nine sites. Most are the trim's own truncations: the dill fixture had lost the property that makes lookup by reference fail, so an otherwise unexplained __module__ = "builtins" sat there with no reason; the rope drift test had two clauses spliced into "every buffer keeps scaling in a buffer"; the playwright guard had lost the three command shapes it matches and why the regex is not anchored on an engine list. Three are a shape I had not seen. The reflow rewrote the `#:` variable documentation markers in the fast-copy constructs to `# :`, and dropped stray colons into the wrapped prose, leaving "text : node". Those markers are syntax, not punctuation to be tidied. One was misplacement rather than truncation. The dotted-import rationale had moved onto the second copy of its case and lost its tail, leaving a dangling "and" above a capitalised "Still caught" and the first row bare. Restored where base has it, stray deleted. lint_duplicate_definitions self-test still passes, 63 cases. * Put five drifted comments back on the code they describe The trim slid several notes onto neighbouring tests, where they describe something they no longer sit on: - test_qat.py: the TODO moved off _test_full_model_fake_quantize, whose leading underscore is what keeps pytest from collecting it, onto a test that is collected and was never disabled. - playwright_chat_ime_i18n.py: 6f carried a stray copy of 6e's compositionstart note above an Enter KeyboardEvent, and had lost its own rationale for why Enter reports isComposing=false while composingRef is pinned. - test_install_node_prebuilt_logic.py: the short-circuit note landed on the transient-download-failure test, leaving the SHA-enforcement test bare and dropping the carve-out that a different usable version is still kept for offline resilience. - test_chat_autoload_failure_gate.py: the default/namespace note moved onto the named-import loop, and the mixed-form note explaining that regex's optional prefix was dropped. - test_frontend_dep_removal.py: the unquoted CSS url() note moved onto the quoted fixture, and the HTML package-name-boundary note moved off the script-src case onto the link-href case. Comments only; the gate reports code unchanged on all five files. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <michaelhan2050@gmail.com>	2026-09-04 15:31:50 -07:00
Leo Borcherding	1dd2fc4583	tests: read checked-in files as UTF-8 instead of the platform default (#7438) ... * tests: read checked-in files as UTF-8 instead of the platform default Path.read_text() with no encoding uses locale.getpreferredencoding(), which is UTF-8 on the Linux runners and cp1252 on a stock Windows install. Nine module-level reads of checked-in source files were relying on that default. studio/backend/routes/inference.py carries the DeepSeek tool-call token regexes, so it holds U+FF5C and U+2581. Under cp1252 that read raised UnicodeDecodeError on byte 0x81 at position 97806, and because the reads run at import time it took test_cancel_atomicity.py and test_cancel_id_wiring.py out at collection, not as failures. Green on CI, permanently broken for a Windows contributor running the suite locally. Adds a guard: at module scope there is no tmp_path fixture, so a bare read_text()/write_text()/open() there is always touching a checked-in file. That makes the rule mechanical enough to enforce with no allowlist, while staying quiet about temp-dir I/O inside test bodies where the platform default is harmless. The repo already spells this correctly in 464 other places; this only stops the stragglers coming back. * tests: cover import-time helper reads and keep the guard py3.9-safe Follows up on the Codex review: - add `from __future__ import annotations`, since `str | None` in `_offender` is evaluated at import on Python 3.9 and pyproject declares requires-python ">=3.9,<3.15". - widen the guard from module scope to import time. Class bodies and the bodies of module-level helpers called from an executing statement run during collection too, so `CODE = _extract_mixed_precision_code()` was the same hazard as an inline read. `if __name__ == "__main__":` blocks are skipped: pytest never executes them. - scan studio/backend/tests/ as well as tests/. Both trees are collected on Windows by separate CI jobs, and the offender that started this, test_tool_xml_strip.py reading routes/inference.py, lives there. Widening it surfaced seven more import-time reads of checked-in sources; all now name utf-8. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Harden the import-time encoding guard for PR #7438 Close the detector gaps raised in review, all of which I reproduced against the actual AST before changing anything. False negatives (the guard let a real hazard through): - _is_main_guard ignored the comparison operator, so if __name__ != "__main__" counted as script-only even though its body runs at import. - The else arm of a main guard was discarded with the rest of the If node. - Decorators and argument defaults on a module-level def were skipped with the body, though both are evaluated when the def executes. - Path.open() in text mode was invisible; only builtin open() was matched. - encoding = None and encoding = "locale" both re-select the platform default, but the keyword merely being present counted as pinned. False positives (the guard would have blocked a compliant contributor): - A non-literal mode fell through to the "r" default, so open(p, mode) was flagged even when mode is "rb", where adding encoding= is a ValueError and there is no edit that satisfies the rule. - Same for open(*args) and a **kwargs splat, which hide the mode and can hide an encoding. - Lambda bodies and comprehension elements were walked even though neither runs at definition. Verified: still reports the same 22 offenders on unpatched main, green on this branch and on the tree merged with latest main (557 files), and an adversarial corpus of 33 cases now scores zero false positives and zero false negatives. Also corrected two docstring claims: neither collecting job runs on Windows, and the read is governed by locale.getencoding(). * Walk eager comprehensions and treat io.open as the builtin Two regressions from the previous commit, both reproduced against the AST before changing anything. Lumping list, set and dict comprehensions in with generator expressions was wrong. Only a genexp is lazy; the other three run their element expression, their filters and their nested iterators immediately, so CONTENTS = [p.read_text() for p in PATHS] at module scope is an import-time read the guard was silently missing. Comprehensions are now walked in full and only the genexp keeps the outermost-iterable-only treatment. io was also in the not-a-path-opener list, but io.open is the builtin, with the same mode position and the same platform default. io.open(CHECKED_IN_FILE) is exactly the hazard this guard exists for, so it is matched now, with binary modes and a pinned encoding still exempt. tarfile.open and fitz.open stay exempt since neither has an encoding to name. Verified: 13 targeted cases covering all five eager comprehension forms and io.open in text, binary and pinned shapes all classify correctly; still 22 offenders on unpatched main; green on this branch and on the tree merged with latest main. * Close three more walker gaps in the import-time guard All three reproduced against the AST first. A generator expression handed straight to a call is consumed there, so DATA = "".join(p.read_text() for p in paths) runs its element at import. Only an unconsumed genexp bound to a name stays lazy, so the walker now follows the consumed ones in full and keeps the outermost-iterable-only treatment for the rest. if "__main__" == __name__ is an equivalent and accepted spelling of the main guard, but requiring __name__ on the left meant its body was treated as import-time code. That is a false positive on a block pytest never runs, so both operand orders are recognised now. The helper table was built from module-level defs only, so a def in a class body invoked while the class is constructed was never followed, contradicting the walker's stated coverage of class bodies. Helpers are now collected from the module body and from class bodies at any nesting. Verified: 15 targeted cases including all three fixes and the earlier ones still classify correctly; still 22 offenders on unpatched main; green on this branch and on the tree merged with latest main. * Handle positional read_text encodings, lazy generators and nested helpers * Guard reads reached from test bodies, unbound Path calls and __file__ paths * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Follow derived paths, skip lazy generator helpers, cover compressed openers * Guard the CLI tests, helper parameters and unbound Path arguments * Discover test roots and follow literal, in-place and tuple-derived paths * Identify module openers by import, unwrap starred paths, pin subprocess snippets * Resolve import origins, seed helper locals, follow named generators and parametrize * Scope imports lexically, list tracked test files, bind unpacked names * Resolve aliased openers, keyword-only params, destructured targets, next() * Pin the encoding on subprocess snippets, workflow lint and CLI output for PR #7438 * Harden the CLI encoding guard against detached streams for PR #7438 * Tighten the encoding guard's path and scope analysis for PR #7438 * Resolve path provenance more precisely and keep POSIX stream encodings for PR #7438 * Resolve qualified path classes and scope conditional imports for PR #7438 * Scope CLI stream setup to the entry point and align two encoding pairs for PR #7438 --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <danielhanchen@gmail.com>	2026-07-26 23:31:56 -07:00
Daniel Han	a963ec92e7	Fix CPOTrainer crash with multimodal processors (Gemma 4) (#6522) ... * Fix CPOTrainer crash with multimodal processors CPOTrainer shares build_tokenized_answer/tokenize_row and __init__ with ORPOTrainer, but the ORPO replacement functions that route tokenization through the underlying text tokenizer and resolve pad_token_id were only registered for orpo_trainer. With a multimodal processing class (e.g. Gemma4Processor) the positional self.processing_class(prompt, ...) call binds prompt to images=, leaving text=None and raising TypeError: 'NoneType' object is not subscriptable. Register the existing orpo_trainer_text_tokenizer and orpo_trainer_processor_pad_token under cpo_trainer as well so CPO/SimPO fine-tuning of multimodal models works. No change for plain tokenizers. * Add CPO processor tokenizer regression test Static, CPU-only checks that cpo_trainer registers the same orpo_trainer_text_tokenizer and orpo_trainer_processor_pad_token rewriters as orpo_trainer, and that the rewriter drops the broken positional self.processing_class(prompt, ...) call. Guards against issue #4952 regressing. * Format CPO test assert for ruff line length (pre-commit) * Bind CPO __init__ pad/eos token reads to underlying tokenizer TRL 0.28+ CPOTrainer.__init__ reads bare processing_class.pad_token and processing_class.eos_token before pad_token_id, which raises AttributeError for multimodal processors (e.g. Gemma) where those live on .tokenizer. Extend orpo_trainer_processor_pad_token to route that block through the underlying tokenizer, and add a regression test. * Tighten code comments (no logic change) * Make CPO/ORPO rewriters reach the trainer on TRL 1.x TRL 1.x moved CPOTrainer and ORPOTrainer out of trl.trainer into trl.experimental.<algo> and dropped the trl.trainer.<algo>_trainer shim that older TRL (0.26 - 0.28) kept. patch_trl_rl_trainers() discovers trainers via dir(trl.trainer), so on TRL 1.x cpo_trainer and orpo_trainer are never found and the multimodal-processor tokenization fix (#4952) silently stops applying, even though the rewriters themselves still match the source. Re-expose experimental-only trainers that Unsloth has rewriters for (RL_FUNCTIONS keys) under trl.trainer before discovery, so the existing patch machinery and its thin-wrapper resolution work unchanged. The alias is a no-op on older TRL where trl.trainer.<algo>_trainer already exists. Also rebind the patched Trainer/Config into every already-imported trl.* module that holds the original class so the fix is visible at the experimental import site (from trl.experimental.cpo import CPOTrainer), not only via trl.trainer. Verified on transformers 4.57.6 + trl 0.22.2, transformers 4.57.6 + trl 0.27.1, and transformers 5.12.1 + trl 1.6.0: CPOTrainer with a multimodal processor tokenizes through the underlying text tokenizer with no crash on all three, and the SFT/GRPO/DPO patch paths are unchanged. * Format for ruff (pre-commit) * Simplify CPO fix to mirror ORPO registrations (#4952) Register the existing ORPO row-tokenizer/pad-token rewriters for cpo_trainer. Under the trl<=0.24.0 pin CPOTrainer lives in trl.trainer.cpo_trainer (found by dir(trl.trainer)), shares ORPO's build_tokenized_answer and uses processing_class.pad_token_id, so the two registrations are sufficient. Drop the trl 1.x experimental aliasing/rebind machinery in rl.py and the bare-pad_token rewriter: trl 1.x (CPO in trl.experimental) and the bare pad_token pattern (trl>=0.28) are not installable under the pin. * CPO: route bare pad_token/eos_token default through inner tokenizer TRL 1.x CPO/ORPO __init__ (the trl.experimental source unsloth resolves on TRL 0.26+) defaults processing_class.pad_token from processing_class.eos_token before tokenizing. Multimodal processors (Gemma3/Gemma4 Processor) expose those attributes on .tokenizer, not on the processor, so that bare access raises AttributeError during __init__ even with the pad_token_id fallback registered. Extend orpo_trainer_processor_pad_token to rewrite that defaulting block to run on the inner tokenizer. The pinned TRL range (<=0.24.0) has no such block, so the regex is a no-op there and only the existing pad_token_id fallback applies. Verified the rewrite against the real trl 1.6.0 experimental CPOTrainer.__init__ (bare access removed, result compiles, a processor without pad_token no longer raises) and added offline regression tests for both the rewrite and its no-op. --------- Co-authored-by: Daniel Han <michaelhan2050@gmail.com>	2026-06-23 06:29:22 -07:00