update capnp to 0.5.3.1 #1322
update capnp to 0.5.3.1 #1322
Conversation
which includes fix for clang 4.x support, matches the pycapnp version used.
|
We're going backwards with the version? |
|
Why would you think so? :) |
|
Never mind, at first read I thought we were going from |
|
@breznak: FYI - he nupic.core/nupic.bindings compilation no longer relies on pycapnp at all. pycapnp is only used by some of the Python tests in |
|
@vitaly-krugl do you approve this PR? |
There was a problem hiding this comment.
@breznak, thank you for your work. Please address feedback concerning pycapnp and spelling of external/CapnProto.cmake in README.
Otherwise, it looks good to me.
| capnproto-c++-win32-0.5.3.zip: from https://capnproto.org/capnproto-c++-win32-0.5.3.1.zip | ||
|
|
||
| When pycapnp (in requirements.txt) is updated, please check with which version of capnproto it ships and update the files also here | ||
| and in external/Capnproto.cmake ! |
There was a problem hiding this comment.
@breznak, why does it matter if pycapnp's capnproto version matches capnproto version here? See additional info in the paragraph below. Also, there might be different reasons for updating pycapnp that would not necessitate update of nupic.core's resident capnproto. If there is still a real reason, please add a comment in the README explaining it. Otherwise, let's remove the reference to pycapnp from this README. The comment describing coordination between the zip names here and CapnProto.cmake is good.
We now pass serialization data via data buffers instead of trying to pass capnproto objects back and forth between potentially-incompatible builds of capnproto in python and nupic.bindings (there used to be ABI issues, incompatible symbol preemption, etc.)
There was a problem hiding this comment.
why does it matter if pycapnp's capnproto version matches capnproto version here?
Sorry, I'm back from holiday, so I may have lost context in here, but the issue with this started from :
capnproto/pycapnp#152 (comment)
So I think the incompatibility was the compiler picked up the (older) capnp version shipped in nupic.core, while newer (by only a x.y.z.1) version was required. I would ask otherwise, do we need this custom installed capnp binary, could we just use the one shipped as dependency of pycapnp?
TL:DR: clang 4.x breaks when these two capnp versions are not in sync, and it isn't documented in nupic.core readmes
There was a problem hiding this comment.
clang 4.x breaks when these two capnp versions are not in sync, and it isn't documented in nupic.core readmes
@breznak, you should be able to build nupic.core without having pycapnp on your system at all. The nupic.core master branch build should only depend on headers and from capnproto that is bundled inside nupic.core (https://github.com/numenta/nupic.core/tree/master/external/common/share/capnproto) and link with archives produced from the same.
Could you please look into why the cmake build breaks with clang 4.x? Perhaps there is some unintentional reference to pycapnp (headers?) accidentally left in nupic.core's cmake configuration that should be removed.
Also, we cannot depend on the capnp .so that is shipped with pycapnp for nupic.core builds, because we have no control over which version of pycapnp (and by extension capnp .so) the user has on their system. The APIs might be incompatible, etc. Also, we cannot use the .so, because we run into duplicate schema ID issue related to how capnproto registers templated schema representations of same schema that is built/used by more than one extension. Capnproto buffer passing was intended specifically to decouple the capnp used by the python layer and the capnp used by the C++ extensions in nupic.bindings and eliminate the various compatibility issues that we faced.
| capnproto-c++-win32-0.5.3.zip: from https://capnproto.org/capnproto-c++-win32-0.5.3.1.zip | ||
|
|
||
| When pycapnp (in requirements.txt) is updated, please check with which version of capnproto it ships and update the files also here | ||
| and in external/Capnproto.cmake ! |
There was a problem hiding this comment.
Spelling - use correct case external/Capnproto.cmake ==> external/CapnProto.cmake
|
@vitaly-krugl Thank you for review! And sorry for the delay. Fixed and commented on your question. |
|
Apparently even newer version is currently used, closing as obsoleted. |
which includes fix for clang 4.x support,
matches the pycapnp version used.
Fixes #1308
Replaces #1318