Skip to content

Publish individual Hugging Face models and pin CI checkpoints - #184

Open
jbohnslav wants to merge 6 commits into
masterfrom
feat/huggingface-models
Open

jbohnslav wants to merge 6 commits into
masterfrom
feat/huggingface-models

Conversation

@jbohnslav

Copy link
Copy Markdown
Owner

Pretrained models now have six individual Hugging Face repositories grouped in a collection, with model cards, configuration summaries, licenses and checksum manifests. The README and setup guide document downloads for DEG-f, DEG-m and DEG-s. Model binaries stay outside this repository.

Test setup downloads the same published checkpoints at immutable commits and verifies their checksums, including when video fixtures already exist. CPU CI and the Modal GPU runner share this setup. New tests check artifact identity, strict parameter loading, finite forward outputs and model discovery for all six models. Video fixtures retain their existing source.

Validation:

  • Full CPU suite: 36 passed.
  • Modal GPU integration suite: 5 passed, covering all presets, feature extraction, sequence training and the softmax workflow.
  • Ruff, pre-commit checks for all changed files, uv lock --check and git diff --check passed.

@github-actions

github-actions Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Claude finished @jbohnslav's task in 20s —— View job


Review of PR #184

The automated /code-review skill errored, so this is a manual review of the code changes: setup_tests.py, modal_gpu_test.py, tests/test_pretrained_models.py, pyproject.toml and the workflow files. I did not run the tests. I also did not read the model cards or docs, or the contents of models.json, SHA256SUMS and the per-model model_config.json. The git diff origin/master...HEAD command also failed with "no merge base", so I read the commit directly.

I found no blocking bugs. The pinned revisions plus the checksum comparison against the checked-in manifests is a sound design. Setup now runs checkpoint preparation even when the video fixtures already exist, which fixes a real staleness gap.

Minor suggestions

  1. Duplicated checksum logic. The checksum verification loop appears in both setup_tests.py:setup_pretrained_models and tests/test_pretrained_models.py::test_published_checkpoint. Extract it into one helper, for example in setup_tests.py, and import it in the test. The two copies could drift apart.
  2. Confusing flow_generator flag. In the test, release["flow_generator"] is truthy for the DEG-f/m/s feature extractors, which produce 700-class logits. model_type is then set to "feature_extractor" when the flag is true. The behavior is consistent, but the name reads inverted. Consider renaming the field in models.json to something like uses_flow_generator, or add a comment.
  3. Truncated downloads. snapshot_download(local_dir=...) leaves a .cache/huggingface folder inside each run directory. This is harmless for discovery, but if a download is interrupted, a partial checkpoint.pt fails only at the checksum step with a generic ValueError. Passing force_download on mismatch, or including the expected and actual digests in the message, would make failures easier to diagnose.
  4. Exit status and error handling. setup_tests() catches every exception and returns False, and __main__ now exits non-zero, which is good. Printing the traceback, for example with traceback.print_exc(), would help CI debugging. At present only str(error) is shown.
  5. Modal volume copy. ignore=lambda directory, names: ... in modal_gpu_test.py compares Path(directory) to VOLUME_ARCHIVE_PATH / "models". This works as long as copytree passes the exact source path, which it does. A short comment noting that the old volume snapshots are intentionally skipped would help.
  6. Dev dependency. huggingface-hub>=0.24 is added to the dev group, and setup_tests.py imports it directly. This is fine, but anyone running it without --dev will hit an import error. The docstring already notes this.

The README and docs changes look consistent with the PR description. I did not verify the Hugging Face repositories or commit hashes against models.json.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-19T14:20:00.983158Z 5411587 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

jbohnslav and others added 5 commits October 19, 2026 14:12
- Rebuild pretrained_models from a separate verified download cache, so
  stale runs or Lightning checkpoints cannot shadow the pinned weights.
- Ship huggingface/ in the Docker test images; a missing manifest now
  fails one test instead of aborting collection.
- Check each v1.0 tag resolves to the pinned commit, share the checksum
  helper, classify models by task, and align models.json flow_generator
  with model_config.json.
- Cache Hugging Face checkpoints in CPU CI.
- Fix flow-model card tags and grammar; update testing and release docs.
- Give the review workflow full history and the plugin's tools.
- Add network-free regression tests in tests/test_setup_tests.py.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- setup_tests.py stamps verified downloads, so reruns skip the Hub and
  re-hashing; touched files are re-hashed and corrupt ones re-downloaded
- Pin a release tag per model in models.json instead of one global tag
- Copy release files into the archive instead of hard-linking, and skip
  rebuilding pretrained_models when it already matches (read-only mounts)
- Docker test images all run setup before pytest
- Replace pytest pythonpath with a conftest that appends the repo root,
  so tests import the installed package
- Share one CI checkpoint cache across OSes; persist Modal checkpoints on
  a writable volume
- Read architecture/task/flow_generator from model_config.json
- Fix the article check in the model card test
- Require pre-commit>=3.2, which pre-commit-hooks v6 needs

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The tests write into tests/DATA (the archive's project_config.yaml and a
project copy), so mounting all of tests/DATA read-only broke them. Mount
the archive read-only elsewhere and copy it in, mount only the verified
checkpoint cache in place, and run setup_tests.py before pytest.

echo_run already runs its command, so drop the duplicate docker run
after each one; smoke tests, GPU checks and pytest were running twice.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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