Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

joeyballentine
Copy link
Contributor

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.

@tencent-adm
Copy link

tencent-adm commented Nov 22, 2023

CLA assistant check
All committers have signed the CLA.

@JeremyRand
Copy link
Contributor

Did you test this with both CPU and Vulkan inference?

@joeyballentine
Copy link
Contributor Author

@JeremyRand I have not tested with CPU. Why would that matter?

@JeremyRand
Copy link
Contributor

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.

@joeyballentine
Copy link
Contributor Author

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

@JeremyRand
Copy link
Contributor

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.

@nihui
Copy link
Member

nihui commented Dec 21, 2023

We may need to add parameters in load_model to distinguish whether ncnn needs to deeply copy the weight data.
This will be available in next year's version :D

@Kim2091
Copy link

Kim2091 commented Apr 12, 2024

Implementing this would benefit many projects. Please consider doing so

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants