| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
Related discussion on the PyPy bug tracker: https://bitbucket.org/pypy/pypy/issues/2434/support-pybind11-in-conjunction-with-pypys |
Sorry, something went wrong.
|
A progress update here:
Cheers, |
Sorry, something went wrong.
|
Nice! That went pretty quickly from "Hello, World" to 95% of the test suite :) Just a quick comment regarding number 6: gc.collect() returns an integer indicating the number of unreachable objects found in that pass. So the following should ensure a full cleanup: while gc.collect():
pass |
Sorry, something went wrong.
It doesn't work. PyPy always returns zero from gc.collect() |
Sorry, something went wrong.
|
I filed a bug report regarding the reference counting leak involving the buffer protocol: https://bitbucket.org/pypy/pypy/issues/2444/pypy-cpyext-reference-counting-leak-when |
Sorry, something went wrong.
|
PyPy bugreport regarding multiple inheritance: https://bitbucket.org/pypy/pypy/issues/2445/support-pybind11-in-conjunction-with-pypys |
Sorry, something went wrong.
|
Hi. PyPY dev here. I am slowly looking into the issues raised. About "PyPy doesn't seem to register class docstrings for some reason" - once PyType_Ready is called, no changes to the c PyTypeObject struct will be reflected into app-level python. About reference counting, we do not support sys.getrefcount() at all and do not promise to collect objects that have been freed. We will not support using del as a mechanism to do anything but free resources. And in more detail: We create an internal app-level object when PyType_Ready is called that reflects the c-level object, we track the refcount of the c-level object and when it goes to 0 may delete both objects at our convenience but our garbage collection is a mark-and-sweep scheme, however if there is no memory pressure we may choose to not collect anything. When we do call the del method on objects, it may be during the execution of exit() from the interpreter. We do not currently have a mechanism to update the app-level object from the c-level object (except in some special cases) after PyType_Ready is called. |
Sorry, something went wrong.
|
Hi @mattip, cool, that's fantastic -- thank you for taking a look. Meanwhile, I've posted a followup to the multiple inheritance issue on the PyPy issue tracker. I've created a new ticket about the issue involving tp_doc. I double-checked, and we're filling out all of the fields before the PyType_Ready call, hence it's strange that the docstrings don't show up in Python: https://bitbucket.org/pypy/pypy/issues/2446/cpyext-tp_doc-field-not-reflected-on The design decisions regarding the cpyext and type construction make sense, and part of the changes in this PR are to make pybind11 deal with that (i.e. not rely on patching up type objects after creating them, which was always kind of hacky in any case) Thanks, |
Sorry, something went wrong.
|
Seeing as PyPy doesn't guarantee cleanup until exit and in general GC behavior can vary based on config and platform, it may be good to disable ConstructorStats checks on PyPy. The destructor counts will likely be unreliable going from machine to machine -- no deterministic way to call gc.collect() the correct number of times. CPython's reference counting mostly plays well with C++'s deterministic destruction, but PyPy has a real GC which doesn't really make any cleanup promises. Perhaps a global option can be added to pytest and guard all ConstructorStats checks (or replace ConstructorStats with a dummy class with a == operator which always returns true). |
Sorry, something went wrong.
|
My impression was that PyPy uses an asynchronous tri-color collector that might not have captured an object that has become garbage in the current iteration (the object might have been referenced at the beginning of that GC cycle), but it will do so in the next one at the latest. Having spent quite a while with this, my practical experience was that two calls of gc.collect() are indeed consistently enough, with the failing testcases turning out to be actual leaks. So I think there is some value in having these, at least until these assumptions are actually violated. |
Sorry, something went wrong.
|
The two main issues (multiple inheritance, docstrings) are now fixed in PyPy 🎉 That just leaves the refcounting issue involving the buffer protocol, and I think we're ready to ship this: https://bitbucket.org/pypy/pypy/issues/2444/pypy-cpyext-reference-counting-leak-when |
Sorry, something went wrong.
|
Slightly off-topic, but I noticed that conftest.py was reflowed to a lower line length. I'm just wondering if this is something that should be added to the flake8 configuration in order to more easily maintain the code style (flake8 is currently set to unlimited line length). |
Sorry, something went wrong.
Agreed. I vouch for linelength of 99 or 100 :) |
Sorry, something went wrong.
@aldanor: this sounds fine. I don't remember why I reflowed that file, probably it did not fit into my arbitrarily sized editor window :) |
Sorry, something went wrong.
|
^ @aldanor, @dean0x7d, @jagerman, @lyskov, @SylvainCorlay, @JohanMabille, @pschella I believe that the few remaining PyPy integration issues will be addressed shortly (the PyPy devs have been super-responsive and already fixed most of the items I reported) This means PyPy support can be part of pybind11 2.0 🎉 . Since it's a somewhat intrusive breaking change (new system and public interface for creating objects with metaclasses and supporting the buffer object protocol), it would be good to have some extra pairs of eyes looking at this PR. Thanks, |
Sorry, something went wrong.
|
|
||
| def collect(): | ||
| gc.collect() | ||
| gc.collect() |
There was a problem hiding this comment.
This function also appears in test_keep_alive.py and test_numpy_array.py. To avoid duplication, it's probably worth adding it to pytest_namespace in conftest.py. Then it can be called like pytest.gc_collect().
Sorry, something went wrong.
|
|
||
| @pytest.mark.parametrize("access", ["static_ro", "static_rw"]) | ||
| def test_property_return_value_policies_static(access): | ||
| test_property_return_value_policies(access) |
There was a problem hiding this comment.
I guess this can be merged back with the test above now that PyPy doesn't need to skip the static part.
Sorry, something went wrong.
| """When returning an rvalue, the return value policy is automatically changed from | ||
| `reference(_internal)` to `move`. The following would not work otherwise. | ||
| """ | ||
| from pybind11_tests import TestPropRVP |
There was a problem hiding this comment.
Same as previous comment.
Sorry, something went wrong.
|
|
||
|
|
||
| def test_roundtrip_with_dict(): | ||
| return False |
There was a problem hiding this comment.
Hm, this test was being skipped but was reported as 'passed' because there were no assertion errors. Should be checked on PyPy.
Sorry, something went wrong.
|
|
||
|
|
||
| def test_docs(doc): | ||
| # PyPy does not seem to propagate the tp_docs field at the moment |
There was a problem hiding this comment.
Wasn't this fixed? Leftover comment?
Sorry, something went wrong.
|
I have tried running my project tests while using this PR code but i am running into what looks like memory error in almost all of them: *** Error in `/usr/bin/python3': free(): invalid pointer: 0x000000000a5b6668 *** ======= Backtrace: ========= /lib64/libc.so.6(+0x7d1fd)[0x7fe440bb21fd] /home/benchmark/rosetta/binder/main/source/build/PyRosetta/linux/clang/python-3.5/minsizerel/build/rosetta.so(+0x15ccef6)[0x7fe42842cef6] /home/benchmark/rosetta/binder/main/source/build/PyRosetta/linux/clang/python-3.5/minsizerel/build/rosetta.so(+0x53d766d)[0x7fe42c23766d] /lib64/libpython3.5m.so.1.0(PyEval_EvalFrameEx+0x655a)[0x7fe4419416ba] /lib64/libpython3.5m.so.1.0(+0x12984c)[0x7fe44194484c] /lib64/libpython3.5m.so.1.0(PyEval_EvalCodeEx+0x48)[0x7fe441944958] /lib64/libpython3.5m.so.1.0(PyEval_EvalCode+0x3b)[0x7fe44194499b] /lib64/libpython3.5m.so.1.0(+0x148d04)[0x7fe441963d04] /lib64/libpython3.5m.so.1.0(PyRun_FileExFlags+0x9d)[0x7fe44196617d] /lib64/libpython3.5m.so.1.0(PyRun_SimpleFileExFlags+0xf7)[0x7fe4419662e7] /lib64/libpython3.5m.so.1.0(Py_Main+0xe94)[0x7fe44197c1f4] /usr/bin/python3(main+0x169)[0x400ad9] /lib64/libc.so.6(__libc_start_main+0xf5)[0x7fe440b56af5] /usr/bin/python3[0x400b89] I will see if i can obtain a better backtrace... |
Sorry, something went wrong.
|
ok on Mac OS, i can see extra message: python(89212,0x7fff79c16300) malloc: *** error for object 0x1256963f8: pointer being freed was not allocated And i got the following backtrace: Catchpoint 2 (exception thrown). Catchpoint 2 (exception caught), throw location unknown, catch location unknown, exception type std::__1::bad_weak_ptr 0x00007fff8e12cac2 in __cxa_throw () (gdb) bt #0 0x00007fff8e12cac2 in __cxa_throw () #1 0x000000010fb101be in dyld_stub_memchr () #2 0x000000010d837d7f in dyld_stub_memchr () #3 0x000000010000e50a in PyObject_Call () #4 0x00000001000192f7 in PyMethod_New () #5 0x000000010000e50a in PyObject_Call () #6 0x00000001000557b0 in _PyObject_SlotCompare () #7 0x0000000100050f87 in _PyType_Lookup () #8 0x000000010000e50a in PyObject_Call () #9 0x000000010008b399 in PyEval_EvalFrameEx () #10 0x0000000100088352 in PyEval_EvalCodeEx () #11 0x0000000100087dcb in PyEval_EvalCode () #12 0x000000010009cee3 in PyImport_ExecCodeModuleEx () #13 0x000000010009fbc2 in PyImport_AppendInittab () #14 0x00000001000a0663 in PyImport_AppendInittab () #15 0x00000001000a022d in PyImport_AppendInittab () #16 0x000000010009e46e in PyImport_ImportModuleLevel () #17 0x00000001000839b3 in _PyBuiltin_Init () #18 0x000000010000e50a in PyObject_Call () #19 0x000000010008e3df in PyEval_CallObjectWithKeywords () #20 0x000000010008a641 in PyEval_EvalFrameEx () #21 0x0000000100088352 in PyEval_EvalCodeEx () #22 0x0000000100087dcb in PyEval_EvalCode () #23 0x00000001000a800e in PyParser_ASTFromFile () #24 0x00000001000a7e2a in PyRun_InteractiveOneFlags () #25 0x00000001000a7939 in PyRun_InteractiveLoopFlags () #26 0x00000001000a77e3 in PyRun_AnyFileExFlags () #27 0x00000001000b9437 in Py_Main () #28 0x00007fff969ac5c9 in start () Please let me know if there is anything else i can do to help |
Sorry, something went wrong.
|
The Python line that lead to this is nothing special: pose = Pose() where Pose class is bound like this: pybind11::class_<core::pose::Pose, std::shared_ptr<core::pose::Pose>> cl(M("core::pose"), "Pose", "...");
pybind11::handle cl_type = cl;
cl.def(pybind11::init<>());
cl.def(pybind11::init<const class core::pose::Pose &>(), pybind11::arg("src"));
cl.def(pybind11::init<const class core::pose::Pose &, unsigned long, unsigned long>(), pybind11::arg("src"), pybind11::arg("residue_begin"), pybind11::arg("residue_end"));
... |
Sorry, something went wrong.
|
Right, of course.. |
Sorry, something went wrong.
|
looks like there is a few new commits now available in wjakob:master. After updating i indeed to longer can see any errors in both Linux and Mac. @jagerman could it be that you missing that example is using pybind11::return_value_policy::reference_internal? |
Sorry, something went wrong.
|
@lyskov - no, the reference_internal isn't the problem, that just uses a keep-alive to delay to Container destruction until after the Data destruction. That part is working: the second Data destruction is happening when Container gets destroyed. |
Sorry, something went wrong.
That's probably just luck, the core problem is still there. |
Sorry, something went wrong.
|
hmmm, ok... then i have questions:
|
Sorry, something went wrong.
|
Also, i was under the impression that object held with reference_internal will never be deleted, - is this no longer true? |
Sorry, something went wrong.
|
Ah, right, I forgot about that. So yes, it is fixed (but you'd run into it again if you returned a pointer). |
Sorry, something went wrong.
|
So, how do i need to bind function that will return pointer to object that should not be deleted? |
Sorry, something went wrong.
|
Scratch what I said, you should be fine with a pointer, too. |
Sorry, something went wrong.
|
ok, i am re-compiling my tests with an updated version of this PR and will post when i have testing results. |
Sorry, something went wrong.
|
For never deleting an object, there are two options:
As for double delete problems, I believe those are now only a risk with the take_ownership policy. |
Sorry, something went wrong.
|
Great, thank you for clarifying this @dean0x7d ! And i will see if i can refactor my code to use py::nodelete instead of T*. |
Sorry, something went wrong.
|
Your T* holder declaration isn't doing anything anyway. |
Sorry, something went wrong.
|
@jagerman i guess it does not now, but it was indeed needed before. A while back binding like: pybind11::class_<B, B* > cl(m, "B_P"); was not compiling without it. |
Sorry, something went wrong.
|
Hmm, I think that right now there is actually no difference between: py::class_<MyClass, std::unique_ptr<MyClass, py::nodelete>>(m, "MyClass")and py::class_<MyClass, MyClass*>(m, "MyClass")Unless I'm overlooking something? |
Sorry, something went wrong.
|
I assume you mean with the T* holder declaration (without it you get a static assertion failure). |
Sorry, something went wrong.
|
Ah, I forgot about the static_assert there. So the holder declaration is not 100% useless, but probably should be avoided. |
Sorry, something went wrong.
|
allright, i just run my set of tests with a new version of source and this time i see no issues so as far as i can tell everything is working! |
Sorry, something went wrong.
|
This is merged now. |
Sorry, something went wrong.
|
Kudos on this one. This is great that it got through. |
Sorry, something went wrong.
|
The readme/docs say:
But I think this should be >= 5.7 (future version, current nightly). Last time I checked 5.6 doesn't even compile. |
Sorry, something went wrong.
|
I wonder if it would be useful to put a static_assert testing for known-unsupported compilers. E.g. it could catch gcc < 4.8, MSVC < 2015 update 3, ICC < 15. And perhaps protected with a PYBIND11_ENABLE_UNSUPPORTED macro that people could test if they are sure they really want to bypass the checks (with the understanding that they are on their own if they do). |
Sorry, something went wrong.
|
@jagerman: sounds like a nice idea! |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
This commit includes a few minor modifications to pybind11 that are needed to get simple hello-world style functions to compile and run on the latest PyPy. Types are now supported as well.
The test suite compiles but crashes when executed.