Unable to use without proto_api despite enable_pyproto_api_setting unset

[Flamefire]

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 of python/google/protobuf/proto_api.h

I made a couple changes such that it builds with the version TF 2.13.0 uses: Flamefire@f49bc41

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.

[rwgk]

@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:

• If you have changes that make pybind11_protobuf usable in your environment,
• Laramie doesn't have objections,
• you document it reasonably well (what I have in mind is a paragraph or two to explain they why and how),
• and add testing,
  we could integrate it (it's a bit tricky technically because we're not set up to import from github, source of truth is Google internal, but I can help with that, it's just a little tedious to manually import; done that before for Add CMake support #73).

[Flamefire]

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.

[rwgk]

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.

[Flamefire]

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: tensorflow/tensorflow#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

pybind11_protobuf/cmake/dependencies/CMakeLists.txt

Lines 18 to 19 in b2e7687

	find_package(Protobuf ${_protobuf_version} EXACT QUIET)
	if(NOT Protobuf_FOUND)

So thanks for adding CMake support here!

[jiawen]

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.

[StefanBruens]

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 ...

See protocolbuffers/protobuf#9464

[Mizux]

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...

[StefanBruens]

Managed to build google-or-tools with distribution packaged protobuf, abseil, pybind11_protobuf and pybind11_abseil. Have just submitted the first corresponding MR here.