Open Flamefire opened 1 year ago
@laramiel do you have suggestions?
However on current main it seems to be much harder as now
check_unknown_fields
depends on that too which makes it look like it may not work that easily anymore.
That code might not be all the interesting externally (outside the Google environment). Do you get further by ifdef-ing that out as well?
Any feedback on the feasibility of the above change/commit?
I don't have a lot of domain specific knowledge (my domain is core pybind11). With that big caveat:
you document it reasonably well (what I have in mind is a paragraph or two to explain they why and how)
What and where do you have in mind? To me it looks rather like a bugfix to an existing feature than something new as that option already exists.
and add testing
Shouldn't that already exist? enable_pyproto_api_setting
is False
by default and I don't see where it is ever set so if this still builds it should be fine, shouldn't it? Or are there some internal test scripts that do set that? In that case it looks like those would need extending for the default case of leaving that unset. Any help here would be appreciated.
Shouldn't that already exist?
I don't know, I can only offer general ideas here since this isn't my domain (see above).
You're ifdef-ing out this include:
#include "python/google/protobuf/proto_api.h"
We have GitHub Actions testing that runs with that include present (it uses the sources as you see on main AFAIK):
https://github.com/pybind/pybind11_protobuf/actions/runs/5869972208/job/15916046871
So something is different apparently between what we have and what you need. My understanding isn't deep enough to know what that could be.
I look around a bit to see if we're doing anything special with enable_pyproto_api_setting
but didn't find anything.
Could you add a new GHA workflow that is representative if your environment and only works with your changes? That would make things more concrete (for me, anyway). Your workflow could use bazel instead of cmake. You may have to piece together the commands to setup bazel instead of cmake. Maybe useful as a starting point, not sure: https://github.com/pybind/pybind11_abseil/tree/master/scripts
I also found this, which runs on an internal build server:
github_init
gcc_init_ubuntu
bazel_init
python_init
bazel build //pybind11_protobuf/...
bazel test \
--test_output=all \
--keep_going \
//pybind11_protobuf/tests:extension_test \
//pybind11_protobuf/tests:message_test \
//pybind11_protobuf/tests:pass_by_test \
//pybind11_protobuf/tests:proto_enum_test \
//pybind11_protobuf/tests:wrapped_proto_module_test
Hope this helps.
Thanks a lot for your replies. However I run into a dead end for now in the use case I intended this for: Compiling TF with a preinstalled protobuf. After patching pybind11_protobuf (linked in the original description) it failed at the end with a protobuf issue I don't see a solution for: https://github.com/tensorflow/tensorflow/issues/61593
I'll come back to this if there is a solution later on for that but until then it doesn't seem to be worth the effort.
Your workflow could use bazel instead of cmake.
FTR: I do like CMake more then Bazel especially as it doesn't require downloading stuff at build time but reusing what is already installed, e.g. at https://github.com/pybind/pybind11_protobuf/blob/b2e7687baf5773cfafcea12d6928991605b14b0d/cmake/dependencies/CMakeLists.txt#L18-L19
So thanks for adding CMake support here!
FYI, I have an approved PR to make proto_api public but haven't had cycles to track down why it would cause CI to fail.
Does anyone have any good idea how to get pybind11_protobuf working again without depending on an in-tree source copy of protobuf?
Unfortunately, upstream protobuf developers seem not to care if they break downstream users, as the promised code has not appeared for 2 years now ...
One dirty solution, would be that pybind11_protobuf directly provide (and install?) the proto_api.h
header in case of cmake based build at least...
Managed to build google-or-tools with distribution packaged protobuf, abseil, pybind11_protobuf and pybind11_abseil. Have just submitted the first corresponding MR here.
I wanted to use this in an environment where the pyext for proto_api wasn't build/installed and as
enable_pyproto_api_setting
is disabled by default I assumed this would work.However the build failed already due to an unconditional dependency on
@com_google_protobuf//:proto_api
and include ofpython/google/protobuf/proto_api.h
I made a couple changes such that it builds with the version TF 2.13.0 uses: https://github.com/Flamefire/pybind11_protobuf/commit/f49bc416cad082d9fb514d4f823f5f597ebffa2e
However on current main it seems to be much harder as now
check_unknown_fields
depends on that too which makes it look like it may not work that easily anymore.Is there interest in getting this fixed/done? Any feedback on the feasibility of the above change/commit?
The usecase was to compile TensorFlow with a pre-installed protobuf to avoid conflicts when using potentially different versions in one environment.