Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix python grpc api build #4791

Merged
merged 2 commits into from
May 10, 2023
Merged

Fix python grpc api build #4791

merged 2 commits into from
May 10, 2023

Conversation

ukclivecox
Copy link
Contributor

  • Creates python folder
  • Creates grpc python bindindings
  • Uses the python grpc-protoc needed for python rather than the usual protoc binary

@ukclivecox ukclivecox added the v2 label Apr 10, 2023
@ukclivecox ukclivecox requested a review from agrski April 11, 2023 07:54
Copy link
Contributor

@agrski agrski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested this with:

cd apis
python3 -m venv venv
source venv/bin/activate
pip install grpcio-tools
make build-v2-python

I've confirmed this generates sensible-looking outputs.


💭 I've made a couple of suggestions for the Makefile.

❓ Separately, elsewhere we've added the generated clients into git. Should we do that here as well?

apis/Makefile Outdated Show resolved Hide resolved
apis/Makefile Outdated Show resolved Hide resolved
mkdir -p python
python -m grpc_tools.protoc \
$(PROTOC_IMPORT_PATH) \
$(PROTOC_PYTHON_OPTIONS) \
--grpc_python_out=./python \
--pyi_out=./python \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 Same here about duplicated info!

Copy link
Contributor

@agrski agrski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more suggested change, but approving for quick merge thereafter

@ukclivecox ukclivecox merged commit 715ad05 into SeldonIO:v2 May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants