fix: explain why a legacy YOLO checkpoint fails to unpickle - #523
Open
roshaninfordham wants to merge 1 commit into
Open
fix: explain why a legacy YOLO checkpoint fails to unpickle#523roshaninfordham wants to merge 1 commit into
roshaninfordham wants to merge 1 commit into
Conversation
yolov5/v7/v9 checkpoints pickle their model classes by reference, so
torch.load only resolves them when the training repository is importable.
Outside it, deploy() failed with a bare
ModuleNotFoundError: No module named 'models'
which names neither the checkpoint nor anything the user can act on.
Translate it into ModelPackagingError, naming the missing module and what
to do about it, and chain the original with `from error`. Other load
failures are left to propagate unchanged.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Partially addresses #357.
That issue reports two failures in sequence. The first —
weights_onlydefaulting toTruein PyTorch 2.6 — is already fixed: everytorch.loadcall inmodel_processor.pynow passesweights_only=False. The second is still live. After downgrading torch, the reporter hit:This is not a roboflow bug in the usual sense, and that is exactly the problem: nothing in the message says so. yolov5, yolov7 and yolov9 checkpoints pickle their model classes by reference (
models.yolo,utils.*) rather than by value, so unpickling only succeeds in an environment where the training repository is importable. Rundeploy()from any other directory and pickle fails on a module the user has no reason to connect to their own checkpoint.Type of Change
Motivation and Context
I could not make the checkpoint load without the training repo — that is inherent to how pickle-by-reference works, and adding a
sys.pathshim would mean guessing where the user's yolov5 clone lives. What is fixable is that the failure gives no indication of the cause or the remedy.So this fails loudly instead of cryptically:
If you would prefer roboflow to attempt the
sys.pathinjection itself, that is a bigger change and a different discussion — I would rather ask than guess. This one is strictly an error-message improvement and changes no successful path.Changes Made
roboflow/util/model_processor.py—_load_checkpointcatchesModuleNotFoundErrorand re-raises it asModelPackagingError, namingerror.nameand the remedy, chained withfrom errorso the original traceback survives. All three call sites (740,1065,1182) go through this helper, so they are all covered.tests/util/test_model_processor.py— aTestLoadCheckpointErrorscase class: the translation, a pass-through of a successful load including the exacttorch.loadkwargs, and a check that unrelated exceptions are not swallowed.Testing
I first confirmed the mechanism against real torch rather than assuming it. Building a checkpoint that pickles a class from a module named
models, then loading it from a directory wheremodelsis not importable:That is the reporter's error exactly. The committed tests then drive
_load_checkpointwith an injected torch double, so they need no torch dependency and no checkpoint fixture — the helper already takes the torch module as a parameter.Against unmodified
mainthe translation test errors with the untranslated exception:Google Colab (optional)
Not applicable; this is an error-path change covered by unit tests, with the underlying mechanism verified locally as shown above.