-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Legacy pickled models support #162
Conversation
# pickled, pruned models. | ||
import models | ||
from models import common | ||
setattr(common, "_Add", _Add) # Definition of the _Add module has moved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hah I love this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was hoping to import it directly in common.py, but quickly found myself in circular import hell. This way is probably more intentional anyway
if "yolov5" not in sys.modules: | ||
sys.modules["yolov5"] = "" | ||
sys.modules["yolov5.models"] = models | ||
sys.modules["yolov5.models.common"] = common |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this does work after all? You were saying it wasn't working at first right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue I was running into with this was actually for the _Add
module. During unpickling it was trying to grab _Add
from models.common
which is not the same as grabbing models.common._Add
. Basically trying to append to an existing namespace like that doesn't work. But creating a new namespace was pretty much painless, just gotta make sure to address every module level (yolov5, yolov5.models, etc..)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
This PR adds backwards compatibility for models that were pickled in a legacy version of our YOLOv5 integration. Three causes of errors are patched here:
load_sparsified_model
was updated to work with a pickled mode_Add
has been moved, which causes unpickling issues. '_Add' has been programmatically added to common, where it is expectedyolov5...
. With the new integration our tools can also be used in the ultralytics upstream repo. Theyolov5
andyolov5.models
namespaces have been added to accomodate loading these legacy models on upstream as well.Test plan
Local testing with legacy model on both installed package through SparseML and directly on fork