* 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>
* 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>
Daniel Han