Support grpc python

[koonpeng]

Even though we currently generate the python grpc files, we do not vendor python grpc so it is not possible to use them without installing external grpc via pip install.

This PR extends grpc_vendor to also vendor python support, the following packages are vendored

• grpcio
• grpcio_tools
• cel (cel pb2 files)
• protobuf

So far I managed to use these vendored grpc packages to connect to flowstate in koonpeng/flowstate-creds-2

sdk-ros/flowstate_credentials_proxy/flowstate_credentials_proxy/__main__.py

Lines 117 to 136 in eb126e8

	channel = create_onprem_channel_async(token_source, cluster)
	async with channel:
	installed_assets_client = InstalledAssetsStub(channel)
	try:
	installed_asset: InstalledAsset = (
	await installed_assets_client.GetInstalledAsset(
	GetInstalledAssetRequest(
	id=Id(package="ai.intrinsic", name="flowstate_ros_bridgea")
	)
	)
	)
	print(installed_asset)
	except grpc.aio.AioRpcError as e:
	if e.code() == grpc.StatusCode.NOT_FOUND:
	# TODO: ask user if they want to install
	raise InitFlowstateRosBridgeError(
	"flowstate_ros_bridge is not installed"
	) from e
	else:
	raise e

[Yadunund]

Deferring to @wjwwood

[wjwwood]

I have to be honest, I don't really like the direction of this pull request, but I think we can make some adjustments to satisfy your needs.

In no particular order here are some of my concerns:

Including grpc python in the grpc_vendor package, is somewhat understandable, but it makes it harder to avoid the diamond dependency problem.

If you already have a python environment and you just want to include some of the things here, you may run into problems due to dependencies like grpc already being in your environment but also being provided by these packages and then running into a conflict. It would be better to source dependencies from a single place. If we want to remove the hack where I set up a python venv just to generate the python code from proto files in intrinsic_sdk_cmake, then I'd prefer to just make it a precondition that python for grpc (etc.) is already in the environment before you try to build intrinsic_sdk_cmake and then require the builder person building the workspace to set up a venv with that stuff before invoking colcon.

cel being part of what intrinsic_sdk_cmake installs doesn't really feel right to me either.

---

If we decide that we want to provide the python packages via our cmake package (i.e. via our colcon build), I would like to see the dependencies in their own _vendor packages so that when we remove the need for them we can cleanly remove them by just excluding/deleting the _vendor packages. I'd also like to see these things sourced via pip, rather than patching/building them locally. We can still pin versions, but pip install X should already work in most cases. We can talk about the best way to do this vendoring, but as I said before, I'd prefer to avoid it if at all possible.

Which may sound hypocritical, however, I think the difference is that compiled languages require us to use the same dependencies as was used to build/generate them, but the generated python code doesn't have the same constraints. As far as I know, having the venv in the build to generate code and then installing the dependencies separately afterwards was working (correct me if that's not true). I think installing the dependencies with pip and a venv afterwards is more "normal", maybe we could set up a requirements.txt file to make it more conventional, but we should avoid providing python dependencies via cmake if we can.

---

Taking a step back, what is the motivation for the pull request?

If it was to make it more convenient to use (no need for venv after build), then I'd say that it will pay off in the long run to source the dependencies normally and that should really be an anti-goal, at least in my opinion. I'm open to be convinced otherwise.

If it was so that you can have cmake/python packages in the colcon build that can use the python code generated from proto files by intrinsic_sdk_cmake, then I'd recommend doing a venv with the dependencies before the colcon build. This way we're still using standard python tooling and we are able to use things like grpc from python in any of our packages, like in intrinsic_sdk_cmake and dependent packages that follow. We could make this easier/more official by either having a requirements.txt file with the dependencies in it, or if we want to go the ROS route, we could use rosdep and new -pip rosdep rules for anything we're missing. Either way we would install the dependencies before doing the colcon build.

If the reason for this pull request was because there was a technical issue with sourcing the dependencies separately from the cmake packages, then let's have a discussion about why that's happening and see if we can resolve it. And if not we cannot resolve it, then let's vendor each python dependency separately and not install things from intrinsic_sdk_cmake and the grpc_vendor packages.

---

I left some inline comments on nitpicks and particular issues, but we should decide on the high level route before worrying about those too much.

[wjwwood]

At the moment this is an ament_cmake package, so I'd recommend using ament_add_pytest_test(), e.g.: https://docs.ros.org/en/foxy/How-To-Guides/Ament-CMake-Python-Documentation.html#using-ament-cmake-pytest

I think it will be more robust that just using python3 here, as depending on the PATH that could point to different things. The Python3 logic in ament is a little more careful about that stuff.

[wjwwood]

Also, I'm not sure this is the best place for this test. ament_sdk_ros is a bit of a placeholder atm, but this test doesn't really have anything to do with ROS. It doesn't even need to be cmake based I think.

Maybe just a small ament_python package with a simple test and a setup.py or pyproject.toml would be better?

[wjwwood]

I think it's weird to include cel here... The other things here are generated from protos. Where is cel coming from anyways?

[koonpeng]

To be more clear, this cel is the proto files for cel. It is already built as part of intrinsic sdk, some of flowstate protos like behavior tree requires this cel protos https://github.com/intrinsic-ai/sdk/blob/e3978e06b311d06444eb9bc2d8941cabd07b6ffa/intrinsic/executive/proto/behavior_tree.proto#L7.

[wjwwood]

Why cython3?

[koonpeng]

python grpc needs cython.

[wjwwood]

We should definitely not use bazel here, I'm actively trying to remove our dependency on bazel.

[koonpeng]

python protobuf does not support building from pure python tools (setup.py). It doesn't look like something that can be easily patched.

[wjwwood]

Please use FindPython (https://cmake.org/cmake/help/latest/module/FindPython.html) and Python_EXECUTABLE or something similar. Don't just rely on the PATH and executable name.

[wjwwood]

nitpick: please use one sentence per line

• [any] Each sentence must start on a new line.
  • Rationale: For longer paragraphs a single change in the beginning makes the diff unreadable since it carries forward through the whole paragraph.

https://docs.ros.org/en/rolling/Contributing/Code-Style-Language-Versions.html#markdown-restructured-text-docblocks

(apologies for the canned response) This is a ROS style guide thing, but I think it makes sense here.

[koonpeng]

The main motivation is to allow building and releasing packages ROS packages that communicate with flowstate. We can use python venv management tools to install grpc but that will not work in the build farm and makes the ROS package not releasable.

What I would like is to allow end users to

sudo apt install ros-jazzy-flowstate-credentials-proxy
ros2 run flowstate-credentials-proxy ...

And also ideally sudo apt install ros-jazzy-flowstate-sdk-python and start building ROS python packages. I think python should have the same level of support as c++ and you shouldn't have to jump out of the ROS ecosystem.

---

I can see if it is possible to vendor prebuilt packages from pypi.

I agree that the vendored grpc conflicting with system grpc is an issue. To address that, I think we can namespace the vendored grpc like what we did with googleapis.

On that topic about conflict, should we also remove GLOBAL_HOOK from grpc_vendor? Correct me if I'm wrong, my understanding of GLOBAL_HOOK is that it will cause the vendored grpc to automatically take precedence over the system grpc if the workspace is sourced.

Happy to discuss other solutions to can allow us to release flowstate packages on the build farm.

[wjwwood]

The main motivation is to allow building and releasing packages ROS packages that communicate with flowstate. We can use python venv management tools to install grpc but that will not work in the build farm and makes the ROS package not releasable.

My opinion on this is that we need to remove the requirement for a newer grpc and stick only to the versions available in Ubuntu to accomplish this. Embedding the dependency in a build farm package is even less desirable than having it in our cmake packages that people build from source. So if that's the end goal, then I think this isn't the path to do that.

I'll ping you and Yadu out of band about what we're doing right now that might help with that.

In the short term, maybe we could have (temporary) pypi packages instead of ROS buildfarm packages for this? It seems like everything you need for that can be done in pure python or at least with things that are already in pypi?

I agree that the vendored grpc conflicting with system grpc is an issue. To address that, I think we can namespace the vendored grpc like what we did with googleapis.

That could be a decent workaround for now, though it means our examples have to use this namespace or we have to do other magic to make it unnamespaced but just for our examples, which sounds fragile, but it could work.

On that topic about conflict, should we also remove GLOBAL_HOOK from grpc_vendor? Correct me if I'm wrong, my understanding of GLOBAL_HOOK is that it will cause the vendored grpc to automatically take precedence over the system grpc if the workspace is sourced.

That is true, but that's what we want right now. We cannot use the system provided grpc right now, which is why we have the vendor package, but my plan to get this on the ROS build farm (which I very much would like to do) would be to remove the need to have any vendor packages at all. Or at least none for dependencies that exist in apt.

Like I said, we can take this out-of-band.