-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Add python binding for loading bin from memory. #5164
base: master
Are you sure you want to change the base?
Conversation
Did you test this with both CPU and Vulkan inference? |
@JeremyRand I have not tested with CPU. Why would that matter? |
When I tried to cherry-pick your binding some months ago and tried it with CPU inference in chaiNNer, I got an immediate segfault. My understanding is that this is because the Pybind deallocates the memory as soon as the bind function returns, which causes a memory safety bug since ncnn doesn't make a copy of the data (since it assumes you're calling from C++ and will manage the memory yourself). I suspect that the reason you didn't see a segfault in Vulkan mode is because the memory management is different (it may make a copy of the data while it's uploading it to the GPU), but I didn't verify this hypothesis. |
Interesting. Well, IMO that isn't enough reason to justify a binding not being there. If it doesn't work for CPU, just don't use it for CPU inference. Sounds to me like it's a bug with the CPU version of the c++ code, and therefore is irrelevant to this PR and should be fixed separately |
Yes, agreed that it's reasonable to have a binding if it works for Vulkan, since it's more efficient than making a copy. If it doesn't work for CPU, might be worth explicitly documenting that, but other than that I have no objection to the concept. |
We may need to add parameters in load_model to distinguish whether ncnn needs to deeply copy the weight data. |
Implementing this would benefit many projects. Please consider doing so |
This PR adds a simple python binding for loading a bin from memory. One already exists for loading a param from memory, and the function to load a bin from memory in c++ already exists, so all that's needed is this binding.
I've been using this exact binding in my ncnn_vulkan fork for a long time now, and it works perfectly fine.
I'm currently considering switching over to the main ncnn package, but I need a couple features (like this) before I am able to do so, so expect some more PRs from me.