ecal
ecal copied to clipboard
Python Binding: Loosen requirement on python-protobuf version
Is your feature request related to a problem? Please describe. Currently, eCAL requires the (more or less) exact version of python3-protobuf that it was built against. The version of the native library is used for that. We should lift that requirement and either specify a manual version (3.0?) or at least give the user the possibility to use a newer version of protobuf. Otherwise, eCAL may conflict with other libraries that require a more recent protobuf version. As python3-protobuf and libprotobuf are independent, they should not conflict.
Describe the solution you'd like Being able to install basically whatever python version I like
Additional context Add any other context or screenshots about the feature request here.
Are you sure that they are truly independent? I guess most of python3-protobuf is a pure python implementation, thus probably not conflicting with eCAL protobuf.
However, we do have generated _pb2.py
files in the wheel (?), and these will not be compatible with another protobuf / protoc version.
I think that we introduced the requirement for a reason.
Hm, ok, so _pb2.py files are obviously not independent from the protobuf version they have been generated with (which is the native protoc, as to my knowledge python3-protobuf does not ship a compiler). What I can tell is that protobuf is usually downwards compatible. eCAL's python samples also ship with pre-generated _pb2.py files that I generated with protoc 3.0 on Ubuntu 18.04. At least those files work with any newer protobuf version I ever tried.
btw, this issue came from an internal request where we simply extracted the whl, removed the maximum protobuf version and repacked it. The wheel worked fine then.
Allegedly some colleagues have even built tensorflow with legacy protobuf versions for making them work with the eCAL Python binding. It's 5h compile time alone for them, we can save them a lot of work by removing the maximum version enforcement
Maybe it makes sense that we have build jobs that can create wheels for a specific protobuf version? Even though protobuf python doesn't use native protobuf, can we make sure tensorflow itself doesn't use it?
Allegedly tensorflow also only specifies a minimum version (>= 3.8). I haven't checked that personally, though. Specifying a minimum version only is common practice with libraries that are usually downwards compatible. I expect the minimum version requirement creating less issues than the current fixed version, as it creates many incompatibilities.
I don't like to have a Matrix [n-python-versions
x n-protobuf-versions
] at all, as all of those wheels would probably only differ in the specific version requirement anyways.
The solution to remove the maximum version requirement has already been tested and worked without issues.
I don't like to have a Matrix [n-python-versions x n-protobuf-versions] at all, as all of those wheels would probably only differ in the specific version requirement anyways.
This is not correct. They would result in ecal_core.so loading a different libprotobuf.so version, as they would build against a different protobuf version. (we would have to change the protobuf submodule for the build).
But, please continue, and maybe we won't run into issues....
Protobuf introduced a incompatibility in a recent version. That recent version is inconsistently referred to as:
- Protobuf 3.21 (protobufcpp)
- Protobuf 4.21 (pypi)
- Potobuf 21 (GitHub Tag)
Either way, it broke compatiblity with old pb2.py files:
At the moment, the most viable solution I can see is to set the required python3-protobuf version to:
protobuf(>=ECAL_PROTO_VERSION,<=3.20)
Obviously, this will create an issue once the ECAL_PROTO_VERSION reaches 3.20 and we will have to take measures for that.
IF (ECAL_PROTO_VERSION < 3.19.0)
protobuf(>=ECAL_PROTO_VERSION,<=3.20.x)
ELSE
protobuf(>=ECAL_PROTO_VERSION)
Corresponding line is here: https://github.com/eclipse-ecal/ecal/blob/b99097ee95aac1c16e6809f07e9dd42917badce3/lang/python/CMakeLists.txt#L38