RosettaCommons / binder

Binder, tool for automatic generation of Python bindings
MIT License
321 stars 67 forks source link

Inheritance problems #28

Closed aalavandhaann closed 7 years ago

aalavandhaann commented 7 years ago

Dear everyone,

I have been trying to pport a C++ library to python and discovered the dynamic duo's Pybind11 and Binder. Though the time to compile binder was longer but still it was worth it.

Getting straight to the problem, I have the following files

X.hpp A.hpp B.hpp

all the above are compiled in the namespace "hello"

//contents of X.hpp
struct X
{
    double x, y, z;
    X();
    X(double x, double y, double z);
};

//contents of A.hpp
#include <vector>
#include <X.hpp>

class A
{
    public std::vector<X> m_contents;
    A();
    void addContents(std::vector<X> contents);
};

void A::addContents(std::vector<X> contents)
{
    for (X item : contents)
    {
        m_contents.push_back(X(item.x, item.y, item.z))
    }
}

//contents of B.hpp
class B: virtual public A
{
    B();
};

in Python (version 3.5)
---------------------------------

>>>from hello import X, A, B;
>>>from random import random;
>>>contents = [X(random(), random(), random()) for i in range(20)];
>>>aa = A();
>>>bb = B();
>>>aa.addContents();
>>>#It compiles fine until this point. After this the sub class instance does the same and crashes
>>>bb.addContents();
>>>Segmentation Fault and crashes as seen in the below message
>>>12139 Segmentation fault      (core dumped) python3

Can anyone point me the silly mistake I am making here? Or it it a genuine issue?

Regards,

0K

lyskov commented 7 years ago

Regarding your example: could you please double check that posted C++ code is matched exactly to what is actually being used? I am asking because in Python you have lines aa.addContents() which should not worked because your C++ function take std::vector<X> as an argument.

Also, do you really need to use virtual inheritance for class B? (ie class B: virtual public A) (The virtual inheritance per se should not be a problem though)

aalavandhaann commented 7 years ago

sorry it was a typo. It should have been

''' a.addContents(contents); b.addContents(contents); '''

But the actual files too have the same structure. Yeah the virtual is not necessary. The cpp is not mine originally. Rather i am making it pythonic for use in blender.

As i was investigating further, the vector member variable works fine on the parent class(A in our case). On the subclass (B.hpp) i can see that the size is in negative value sometimes.

Regards,

0K

lyskov commented 7 years ago

I would say that this example should work out-of-the box, - i do not see anything wrong with it.

As an idea for investigation i would suggested to:

If after all that you continue to see crashes i would recommend to try to extract minimal code that trigger crashes and post in on Pybind11 issues since reproducing this case should be take only small amount of code.

lyskov commented 7 years ago

Also, just to double checked: how did you build contents? I mean the Python code that you posted above will produce Python list, not the std::vector.

Also related: how did you bind std::vector? Did you use helper functions that we ship with Binder for that?

aalavandhaann commented 7 years ago

Lyskov,

Firstly, thanks to you. Indeed removing the 'virtual' keyword helps. I tested the code and there was no segmentation fault. Many thanks to your advice.

Regarding the 'addContents': The 'contents' list will have 20 elements of X. If you look closely then its a list instantiated with 20 instances of X. To bind the std::vector to a list I specified that stl.h be included inside the config file using +include <pybind11/stl.h>.

When I tried using the shipped one there were some errors which I honestly didn't observe as I was trying to run the library first. So using <pybind11/stl.h> made it work. When trying to use stl_binders.hpp shipped binders I gave the following line in the config:

+include <python/PyRosetta/binder/stl_binders.hpp>
+binder std::vector binder::vector_binder

The above lines failed for some reason which I haven't taken a note of yet. Maybe I will post the errors before evening.

Regards,

0K

aalavandhaann commented 7 years ago

Dear Lyskov,

I tried using the shipped stl_binders. Yes it works but there is one thing worth mentioning at this point. In my conf file

+include <stl_binders.hpp>
+binder std::vector binder::vector_binder

The above entries did not enable the conversion of std::vector to python lists automatically. But after adding the <pybind11/stl.h> to the config it works fine. The final entries of my conf reflecting a proper working library

+include <pybind11/stl.h>
+include <stl_binders.hpp>
+binder std::vector binder::vector_binder

I am head over heels in love with this tool.

Regards,

0K

lyskov commented 7 years ago

Glad to hear it works now! I am going to close this issue then.

Also, re std::vector: this is expected behavior: such automatic conversion between Python list and std::vector while handy come at the price of generating new std::vector on-the-fly which might not be desirable due to performance cost.

aalavandhaann commented 7 years ago

Thanks for closing this issue. Do you think it is better to mention this 'virtual' keyword thing in the docs? Or is it better to notify this to the pybind11 guys? I believe this is more of a limitation of pybind11 rather than binder itself. After all binder only generates the binding code.

Apart from posting issues, is there a place to do discussion about this wonderful tool? I would love to participate. Also I am finished with the library. Testing it rigorously. I would be more than happy to submit the source code for demo'ing an example apart from the examples posted here. Hope the tests go well. Cheers and thanks a lot.

Regards,

0K

lyskov commented 7 years ago

@aalavandhaann revirtual keyword: this is unexpected and probably a bug (Pybind11?). I know for sure that bindings for virtual classes is working correctly for PyRosetta project so it is not like whole thing is broken. If you can (and that will be really helpful!) it would be great if you can draft minimal example that trigger this problem so we can submit it to Pybind11 team.

Re discussion: I believe GitHub right now is the only place.

Re demo: that would be great! Would you be interested in putting together short document that outlined what steps you take to create bindings for your project? If so, I would be happy to put a link to it into Binder docs.

aalavandhaann commented 7 years ago

@lyskov Thank you for all the help. In fact I have created another ticket about distribution of a binder project and closed it myself with the solution found. I am still not complete with handling the challenges and handling all the goals of my project.

I was able to create a PyPi package successfully and also was able to test on 3 ubuntu machines (using pip install). The only issue I found was that the if gcc version < 4.9 then the std=c++14 flag is problematic. But this is solved when the user upgrades his gcc compiler to a version >= 4.9.

I am yet to test it on mac machines. As of now my struggle is with windows machines where the pip install fails due to errors. I will now create a ticket for windows. Once I nail this my observations and solutions are already being documented to a text file will be compiled into a readable document. I will compile it as a rst file or pdf and will send it to across. Of course this is the minimum I could do to ease the load you guys.

Regarding the minimal example to investigate about 'virtual' keyword, yes, I will create a project sample and verify myself where the issue is. I need to know if it was coming from my existing codebase or 'virtual' is a problem with pybind11 itself.

Also I am curious to know if Binder will move forward to the newest version of pybind11 where 'MODULE' is the way to adding packages instead of 'PLUGIN' that is deprecated now.

Regards,

0K

lyskov commented 7 years ago

Re moving to use new MODULE macro: yes, this is already in the work: right now i working on updating Binder so it could use latest Pybind11-2.2. This might take some time though due to critical bugs in Pybind11 which will need to be fixed first.