3 commits

Author	SHA1	Message	Date
FrMakeThisWorkF	259ec3b22d	Fix formatting_func race in UnslothVisionDataCollator without a lock (#7698) ... * fix: add thread-safety to UnslothVisionDataCollator * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix the race without a lock, and make the test actually catch it for PR #7698 The test patched __call__ on UnslothVisionDataCollator itself rather than on the zoo base, so collator(...) dispatched straight to the fake and the real __call__ never ran. It passed with the fix reverted, and with the body replaced by an unconditional raise. It now patches the zoo base like the three tests above it, parks a leader inside the mutation window and releases 31 followers behind it, and asserts every example is formatted exactly once. Verified: fails with "assert 3 == 96" on the pre-fix body, passes 10/10 with the fix. It also imported unsloth.trainer directly instead of taking the real_collator_classes fixture, so it errored on windows-latest where triton has no wheel while every other test in the file skipped cleanly. The lock is replaced by a per-call shallow view. Holding it over the whole collate was forced by mutating self (a narrow lock still races, since the base reads formatting_func for the entire call), and it cost more than the race: threading.Lock is unpicklable, so deepcopy and cloudpickle of the collator stopped working, which breaks Trainer.hyperparameter_search(backend="ray") because Ray ships the whole Trainer through its object store. It also deadlocked a re-entrant formatting_func, and a fork taken while another thread held it left the child hung. copy.copy on a __slots__ object shares processor, size_func and _checked_video_paths by reference, so only formatting_func differs on the throwaway view and no concurrent caller sees a mutated self. Losses, grad norms and peak memory are identical over 20 steps of a Qwen2-VL-2B LoRA run, and all 36 tests in the file pass. * Skip the real-collator tests on the MLX placeholder for PR #7698 On Apple Silicon, unsloth/__init__.py installs a shim for unsloth.trainer whose UnslothVisionDataCollator is a placeholder that raises NotImplementedError, not the zoo subclass. The fixture imported it successfully and handed it over, so test_real_collator_blocks_super_on_missing_video and test_real_collator_calls_super_with_formatting_disabled have been failing on macos-14 on main, and the rewritten thread-safety test joins them because the placeholder never reaches the patched base. Guarding the fixture on issubclass turns all three into clean skips, matching what the same tests already do when the import fails outright. Confirmed against a stubbed placeholder: 4 skipped instead of 3 failed, and the torch path is unchanged at 36 passed. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Daniel Han <danielhanchen@gmail.com>	2026-08-02 22:44:09 -07:00
Daniel Han	a6dc10dad2	Reduce and tighten comments and docstrings across the test suite (#6429) ... * Reduce and tighten comments and docstrings in tests Shorten verbose comments and docstrings across the test suite without changing any test logic. Remove narration that restates the next line, collapse long module and test docstrings to a single line, and drop banner separators. Keep regression context (issue and PR references, run ids), skip reasons, mocking and timing rationale, license headers, lint and type directives, and commented-out code. Comments and docstrings only: an AST signature check confirms no code, assertions, or string literals changed, and the suite byte-compiles cleanly. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-06-18 01:07:09 -07:00
Leo Borcherding	756e129388	fix/validate dataset video paths before training (#5136) ... * fix(vision): validate dataset video paths before training * fix(vision): remove redundant warnings import, add pytest tests for #5085 * fix(trainer): auto-validate video paths in UnslothVisionDataCollator on first batch (#5085) * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * refactor(vision): use str.removeprefix instead of slicing (Datta0 review) * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * style: replace em dashes with hyphens in error message and docstring * Update unsloth/models/vision.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Split: keep only 1 file(s) * fix(vision): broaden video-path validation to all collator inputs - check_dataset_for_missing_videos now accepts every example shape that UnslothVisionDataCollator forwards to process_vision_info: dict rows with "messages"/"conversations"/"prompt"/"completion", and raw message-list rows. Earlier logic only handled {"messages": [...]}, so conversations/prompt/completion datasets silently skipped validation and raw message-list rows crashed on list.get. - Guard against non-dict message entries; a bare string inside a message list no longer raises AttributeError. - Decode file:// URIs via urllib so percent-encoded paths, absolute Windows URIs (file:///C:/...) and host-qualified URIs (file://localhost/abs/path) map back to their real filesystem path. - Expose an optional "checked" set so callers can reuse dedup state across invocations. - Docstring warns that passing a streaming IterableDataset consumes the iterator. UnslothVisionDataCollator now validates every batch (not just batch 0) and applies formatting_func before validation, matching the base collator's own ordering so formatter-generated video paths are also checked. The already-checked set is shared across batches, so per-batch cost stays proportional to newly seen paths. * fix(vision): robust URI + scheme handling in video-path validator - _local_path_from_video_value now treats anything with a "://" prefix as a URI and validates only file:// URIs. This prevents false FileNotFoundError on remote schemes that were silently passed through before (s3://, gs://, hf://, ftp://, az://, ...). - Non-localhost file authorities (e.g. file://nas-server/share/clip.mp4) are now skipped instead of being stripped and validated against the local filesystem; RFC 8089 only permits empty host or "localhost" for local files. - Drop the explicit unquote call: urllib.request.url2pathname already unquotes, so the previous url2pathname(unquote(path)) double-decoded any filename with a literal percent (e.g. a file named "clip%20.mp4"). - Remove the Windows drive-letter strip block; nturl2path.url2pathname handles "/C:/foo" -> "C:\\foo" itself, leaving nothing for the guard to match on either OS. - Return None when the resolved path is empty (bare "file://" or "file://hostname") so the caller skips it instead of reporting a blank " - " entry in the error message. - Add a runtime guard in check_dataset_for_missing_videos that warns and returns early when handed a datasets.IterableDataset, matching the docstring contract and preventing silent iterator exhaustion. Windows native paths like "C:/path/x.mp4" stay valid because the scheme check uses the "://" substring (not urlparse's single-letter scheme surface). * tests: consolidate video-path validation coverage into one file New coverage for tests/test_video_path_validation.py: - every-batch validation with cross-batch dedup (replaces the old first-batch-only assertion which no longer matches the implementation). - all collator-supported input shapes: messages, conversations, prompt/completion, raw-message-list rows; non-dict message entries. - file:// URI robustness: percent-encoded paths, localhost netloc, non-localhost netloc skipped, bare / hostname-only URIs skipped, double-encoded filenames single-unquote correctly. - non-file remote schemes (s3, gs, hf, ftp, az) skipped without raising. - Windows-style absolute path not mistaken for a URI scheme. - formatting_func applied before validation inside the collator wrapper. - IterableDataset runtime guard warns and returns without consuming. The pre-existing test_collator_validates_only_once assertion has been replaced by test_collator_validates_every_batch / dedupes_across_batches because the wrapper now validates every batch. The session-scoped AST fallback fixture was extended to extract the helper functions that the rewritten check_dataset_for_missing_videos depends on, so the Windows/no-triton code path still loads the module surface. * Fix CI iter 1 * Omit release-desktop.yml from the PR diff The workflow file landed on origin/main after the PR branched off; our fork-scoped push token cannot touch .github/workflows/**. Drop it from this branch so the PR diff stays within the author's authorisable surface. The file remains on origin/main and will return after this PR merges upstream. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix(vision): cache only validated paths in checked set Missing paths were added to the dedup cache before the existence check, so a caller that caught FileNotFoundError and retried with the same collator/checked set would silently skip the bad path on the second call. Only add a path to checked after os.path.isfile confirms it exists, so missing paths are re-validated on every call until they are fixed. * fix(ci): resolve two CI failures introduced by this PR - Add __all__ to models/__init__.py so the HOISTED-IMPORT-UNUSED linter check passes for check_dataset_for_missing_videos - Replace Dataset.from_list() in test helpers with plain list literals; the CI environment mocks datasets with a MockDataset that only has from_dict, but check_dataset_for_missing_videos accepts any iterable so no Dataset wrapper is needed - Guard test_iterable_dataset_warns_and_skips with pytest.importorskip so it skips cleanly when the real datasets package is unavailable * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix(ci): correct HOISTED-IMPORT-UNUSED without breaking wildcard exports Adding __all__ to models/__init__.py was too aggressive - it restricted from .models import * in _gpu_init.py to only check_dataset_for_missing_videos, hiding FastLanguageModel, FastVisionModel etc and breaking test_fast_model_class_surface_under_spoof. Instead: remove __all__, and add an explicit named import in _gpu_init.py so the linter sees the symbol consumed in the re-export chain. * Deduplicate missing video paths and skip data URIs in validator check_dataset_for_missing_videos appended a path to the missing list on every occurrence, so a path referenced by multiple rows was reported N times and the error header read the wrong count. Track missing paths in a per-call set so each is listed once, kept separate from the checked cache so retries still re-check missing files. This restores the dedup behaviour the docstring promises and the existing test_duplicate_paths_deduplicated test asserts. Also skip data: URIs in _local_path_from_video_value so inline base64 payloads are not flagged as missing files. Add tests for the data URI case, warn-only dedup, and real integration against the unsloth_zoo UnslothVisionDataCollator base (verifying validation gates the base call, formatting_func is applied once and restored even when the base raises). * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix(trainer): declare __slots__ on UnslothVisionDataCollator subclass * fix(ci): hoist check_dataset_for_missing_videos to trainer module level; guard IterableDataset skip * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix(vision): accept tuple message content in video path validator * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Tighten comments and docstrings for PR #5136 Comment-only pass over the new video path validation code: shorten the collator and validator docstrings, collapse multi-line inline comments, and reduce test docstrings to one-liners. No code changes; verified with comment_tools.py check --strip-docstrings (3/3 code unchanged) and the full test suite (35 passed). * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Daniel Han <danielhanchen@gmail.com>	2026-06-10 05:32:32 -07:00