Auto-download YOLO-NAS weights; drop L2CS from gated nightly#324
Conversation
![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: d1d34730a4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # than hard-failing on a missing file. | ||
| from ...utils.download import download_weights | ||
|
|
||
| download_weights(model_path, self.size) |
There was a problem hiding this comment.
This new auto-download path still passes paths like downloads/yolonas/yolo_nas_s_coco.pth from the general nightly matrix into download_weights, but that helper chooses a URL by calling cls.get_download_url(Path(model_path).name). LibreYOLONAS.get_download_url() only recognizes canonical LibreYOLONAS{s,m,l}.pt names via the base filename regex, not Deci's upstream yolo_nas_s_coco.pth, so clean nightly/user runs without hand-staged YOLO-NAS downloads still cannot determine a URL and will skip/fail instead of provisioning the weight. Route by self.size here or teach the URL resolver to recognize the upstream filenames.
Useful? React with 👍 / 👎.
Make the gated nightly cover every public family except the
non-redistributable L2CS/Gaze360, and fix YOLO-NAS auto-download end to end.
YOLO-NAS changes (libreyolo/models/yolonas/model.py):
- detect_size_from_filename now also parses the native Deci filenames
(yolo_nas_<size>_coco.pth) in addition to the LibreYOLO convention, so
get_download_url can resolve the size and return the Deci CDN URL. Without
this it returned None and download was impossible.
- _load_weights now calls download_weights() on a missing file instead of
raising immediately, like the base loader -- so users and the nightly fetch
the checkpoint from Deci's CDN on demand (the weights are not mirrored on
the LibreYOLO HF org by license).
Test gate (tests/e2e/conftest.py): require_test_weights only auto-passed
huggingface.co/LibreYOLO routes, so YOLO-NAS skipped (-> nightly failure)
before the model could download. Accept the upstream hosts utils.download
knows how to fetch from (Deci CDN) too.
Drop l2cs-r50 from GENERAL_NIGHTLY_INFERENCE_MODELS: Gaze360's license
forbids redistribution and it has no plain-HTTP route, so a skip-means-failure
gate cannot provision it. Gaze stays covered by the non-gated per-family L2CS
suite. Bump the nightly contract to 2.0 (narrower promise).
Verified on a CUDA runner: yolonas-s cold-downloads from Deci and passes
deterministic inference; l2cs-r50 no longer collected; yolo9-t still passes.
EHxuban11
force-pushed
the
fix/nightly-drop-l2cs-yolonas-autodownload
branch
from
d1d3473 to
75d98da
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75d98da1eb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| match = re.search(r"yolo_nas(?:_pose)?_([nsml])_coco", filename.lower()) | ||
| return match.group(1) if match else None |
There was a problem hiding this comment.
For missing YOLO-NAS pose weights named with Deci's native convention, e.g. yolo_nas_pose_s_coco_pose.pth, this new regex now makes get_download_url() think the file has a known size, but detect_task_from_filename() still only understands the LibreYOLONAS*-pose.pt convention. The task is therefore None, so the downloader returns yolo_nas_s_coco.pth and _load_weights() later fails because a detection checkpoint was fetched for a pose model. Add native pose task detection or special-case these filenames before returning the URL.
Useful? React with 👍 / 👎.
| if url and ( | ||
| url.startswith("https://huggingface.co/LibreYOLO/") | ||
| or "cloudfront.net" in url | ||
| or ".deci.ai" in url |
There was a problem hiding this comment.
When test_missing_local_only_weight_path_skips runs, this new Deci host allow-list makes require_test_weights("downloads/yolonas/yolo_nas_s_coco.pth") return the path instead of raising pytest.skip, so the existing assertion in tests/e2e/test_weight_requirements.py now fails. Update that test to reflect that YOLO-NAS is auto-downloadable, or keep this preflight helper treating the path as local-only.
Useful? React with 👍 / 👎.
| # L2CS/Gaze360 is intentionally excluded: its license forbids redistribution and | ||
| # it has no plain-HTTP route, so a skip-means-failure gate cannot provision it. | ||
| # Gaze inference stays covered by the non-gated per-family L2CS suite. |
There was a problem hiding this comment.
After this removal, repo-wide search shows the only e2e L2CS path was the family == "l2cs" branch in tests/e2e/test_deterministic_inference.py, which is driven solely by GENERAL_NIGHTLY_INFERENCE_PARAMS; there is no separate non-gated L2CS e2e parametrization. As a result, removing L2CS from this list drops gaze inference from e2e entirely despite the new comment/contract saying it remains covered, so add the non-gated per-family case before excluding it from the gated matrix.
Useful? React with 👍 / 👎.
- yolonas.detect_task_from_filename now recognizes Deci's native pose filenames (yolo_nas_pose_<size>_coco_pose.pth), so get_download_url routes a pose request to the pose checkpoint instead of fetching detection weights that then fail the pose/detection guard. (P2) - test_weight_requirements: YOLO-NAS's native path is now auto-downloadable via Deci's CDN, so assert it resolves a route; switch the local-only skip case to a genuinely route-less weight (downloads/l2cs/LibreL2CSr50.pt). (P2) - Add tests/e2e/test_l2cs_gaze.py: a non-gated (e2e, NOT general_nightly) gaze stability check so dropping L2CS from the gated matrix does not remove gaze coverage entirely. It runs when the non-redistributable weight is staged and skips cleanly otherwise (a skip here is never turned into a failure). (P2) Verified on a CUDA runner: pose vs detect URLs route correctly; the 4 preflight tests pass; the L2CS test skips cleanly (not failed) under LIBREYOLO_FAIL_ON_NIGHTLY_SKIP=1; yolonas-s still passes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d30b41ce27
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # than hard-failing on a missing file. | ||
| from ...utils.download import download_weights | ||
|
|
||
| download_weights(model_path, self.size) |
There was a problem hiding this comment.
When a YOLO-NAS weight is missing, this new call fetches a third-party .pth from Deci/CloudFront and the same method then loads that file with torch.load(..., weights_only=False). In the auto-download path this turns a network response into unrestricted pickle execution without a hash or the existing load_untrusted_torch_file safety wrapper, so a compromised CDN/object would execute code during model construction rather than just failing to load weights.
Useful? React with 👍 / 👎.
| "general=smallest native inference case for every public detector family, " | ||
| "each pulled from a public auto-download route (LibreYOLO HF, or Deci's CDN " | ||
| "for YOLO-NAS); gaze (L2CS/Gaze360) is non-redistributable and runs only in " | ||
| "the non-gated per-family suite, not the gated nightly; " | ||
| "flagship=heavier native YOLO9/RF-DETR validation, training, video, " |
There was a problem hiding this comment.
This contract now says L2CS is only in the non-gated suite, but docs/testing.md still documents the V1 nightly contract as general_nightly covering every public family with L2CS gaze coverage and still lists weights/LibreL2CSr50.pt among missing local weights. Because the repo instructions treat /docs/testing.md as the CI contract, maintainers following the docs will expect a different gated suite than the one this version string advertises.
Useful? React with 👍 / 👎.
| import torch | ||
| from PIL import Image | ||
|
|
||
| pytestmark = pytest.mark.e2e |
There was a problem hiding this comment.
This new replacement for the removed matrix coverage is only marked e2e, so per-family selection with pytest -m l2cs still won't run any L2CS e2e inference despite the surrounding contract calling this the non-gated per-family L2CS suite. Add the l2cs marker here (or use family_marks) so targeted family jobs and local validation actually include the staged-weight gaze check.
Useful? React with 👍 / 👎.
| from libreyolo import LibreL2CS | ||
|
|
||
| device = "cuda" if torch.cuda.is_available() else "cpu" | ||
| model = LibreL2CS(weights, size="r50", device=device) |
There was a problem hiding this comment.
When this new e2e check runs on a GPU with staged L2CS weights, it constructs the model and runs two passes but never deletes it or calls the existing cuda_cleanup() helper; the previous general-nightly branch did clean up immediately after L2CS. In full e2e runs this can leave CUDA allocations cached for later heavy tests in the same process, making staged-weight jobs more prone to OOMs.
Useful? React with 👍 / 👎.
…docs - yolonas: auto-downloaded checkpoints are third-party pickles loaded with weights_only=False (YOLO-NAS checkpoints require it), so verify them against a pinned SHA-256 of the official Deci CDN files before unpickling. Unknown or mismatched files are deleted and refused, so a compromised CDN object fails closed instead of executing code during construction. Locally staged files are the user's own trust decision and are not re-verified. (P1) - test_l2cs_gaze: add the per-family l2cs marker so pytest -m l2cs runs the gaze check, and free CUDA memory via cuda_cleanup() after inference. (P2) - docs/testing.md: sync the CI contract to V2 (general_nightly = 15 detector cases with a public auto-download route; L2CS gaze moved to a non-gated per-family suite; YOLO-NAS auto-downloads, so it is no longer a missing local weight). (P2) Verified on a CUDA runner: checksum accepts the official file, refuses unknown and tampered files (deleting them); pytest -m l2cs selects the gaze test; yolonas-s still downloads, verifies, and passes inference.
|
@cursor review |
|
@codex review |
|
@claude review |
There was a problem hiding this comment.
💡 Codex Review
libreyolo/libreyolo/models/yolonas/model.py
Lines 98 to 99 in 3108cdb
Don't advertise unpinned YOLO-NAS pose downloads
For missing YOLO-NAS pose weights, this branch returns a public CDN URL, so download_weights() and the e2e preflight both treat LibreYOLONASs-pose.pt / yolo_nas_pose_s_coco_pose.pth as auto-downloadable. The verifier added below only pins detection filenames, so after the pose file is downloaded it is immediately deleted and loading raises no pinned checksum is known; either add pose checksums or avoid returning a route for pose so callers get a clear manual-staging path instead of a broken auto-download.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| name = Path(model_path).name | ||
| expected = cls._DECI_CHECKPOINT_SHA256.get(name) |
There was a problem hiding this comment.
When a user requests the normal LibreYOLO filename, e.g. LibreYOLONASs.pt, BaseModel resolves it to weights/LibreYOLONASs.pt and the new download path fetches Deci's yolo_nas_s_coco.pth into that destination. This line then checks the destination basename (LibreYOLONASs.pt) against a pin table that only contains native Deci names, so it deletes the freshly downloaded file and raises before loading; the advertised LibreYOLO filename auto-download route therefore fails unless callers use the native yolo_nas_*_coco.pth name.
Useful? React with 👍 / 👎.
| download_weights(model_path, self.size) | ||
| self._verify_downloaded_checkpoint(model_path) |
There was a problem hiding this comment.
This checksum gate only runs when LibreYOLONAS is instantiated directly with a missing path, but the public LibreYOLO(...) factory downloads missing files before class selection and then passes metadata-less upstream checkpoints as an extracted state dict, so _load_weights() and this verification call are skipped on the path used by the new nightly case (LibreYOLO(weights, ...)). That leaves the advertised checksum-verified YOLO-NAS auto-download unverified in the main API path, or it fails earlier during the factory's safe inspection instead of using the verified YOLO-NAS loader.
Useful? React with 👍 / 👎.
Bugbot couldn't runSomething went wrong. Try again by commenting "Cursor review" or "bugbot run", or contact support (requestId: serverGenReqId_49c6ef59-fa4b-46ca-9072-d870d8e8bf3c). |
… name
The previous checksum gate lived in LibreYOLONAS._load_weights and keyed on
the destination filename. That had two holes Codex flagged:
- The public LibreYOLO(...) factory downloads a missing file itself (before
class selection) and does not route through the per-family _load_weights, so
the verification was bypassed on the main API path (and the nightly's
LibreYOLO(weights, ...) call).
- Keying on the destination name broke the LibreYOLO-convention route: a user
passing LibreYOLONASs.pt saves Deci's yolo_nas_s_coco.pth bytes to
weights/LibreYOLONASs.pt, which is not in the (native-name) pin table, so the
freshly downloaded file was deleted and refused.
Move verification into download_weights -- the single chokepoint every download
(factory and per-family loaders) funnels through -- via a new
BaseModel.verify_downloaded_file(local_path, source_url) hook (no-op default;
LibreYOLONAS overrides it). It keys on the SOURCE url basename, so both naming
conventions verify against the right pin and the gate cannot be bypassed.
Verified on a CUDA runner: download_weights calls verify exactly once, keyed by
yolo_nas_s_coco.pth even when the destination is weights/LibreYOLONASs.pt;
LibreYOLO("weights/LibreYOLONASs.pt") downloads, verifies, and infers; the
nightly yolonas-s case still passes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f008f89a3f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| _DECI_CHECKPOINT_SHA256 = { | ||
| "yolo_nas_s_coco.pth": "c1b1d9148ab8ae5d5984699547e850955ff9efccaf568c67b3d605acb4bfe1cb", | ||
| "yolo_nas_m_coco.pth": "b194fc7fa196f76161c6356558bedf04fb99a62325a74a36a4bec3ca8ba48250", | ||
| "yolo_nas_l_coco.pth": "91a06beaa1ce1a651d6691e3198061da996eafc8890503238dedacbd4c392a32", | ||
| } |
There was a problem hiding this comment.
In this version the filename resolver now returns Deci's pose CDN URL for missing LibreYOLONAS*-pose.pt or yolo_nas_pose_*_coco_pose.pth files, but the verification table only pins the three detection checkpoint names. For those pose requests, verify_downloaded_file() sees no expected digest, deletes the freshly downloaded pose file, and raises before loading, so the new auto-download route fails specifically for YOLO-NAS pose weights; add the pose hashes or avoid returning pose URLs as auto-downloadable.
Useful? React with 👍 / 👎.
|
@cursor review |
…wnload get_download_url returned Deci's pose CDN URL for pose filenames, but only the three detection checkpoints (s/m/l) have pinned SHA-256s. So a pose request resolved a route, downloaded, then verify_downloaded_file deleted it and raised for the missing pin -- breaking pose auto-download with a confusing download-then-refuse. Scope YOLO-NAS auto-download to the pinned detection checkpoints: return no route for pose filenames so they fail fast with 'could not determine download URL' and must be staged manually, rather than fetching an unverifiable pickle. Task detection is still applied, so a pose request never mis-routes to the detection URL. (Pose was not auto-downloadable before this PR either.) Verified on a CUDA runner: detection names resolve the Deci URL; native and LibreYOLO-convention pose names resolve url=None and fail cleanly without a download-then-refuse.
Bugbot couldn't runSomething went wrong. Try again by commenting "Cursor review" or "bugbot run", or contact support (requestId: serverGenReqId_51dcab7a-7f53-4c8c-b666-a31125b79c4f). |
1 participant
No description provided.