-
Notifications
You must be signed in to change notification settings - Fork 100
Conversation
python/gubernator/gubernator_pb2.py
Outdated
@@ -15,552 +14,41 @@ | |||
from google.api import annotations_pb2 as google_dot_api_dot_annotations__pb2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd note that this import doesn't appear to be used anymore, not sure what's happening there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! Have only constructive criticism to work out and then we can merge.
python/README.md
Outdated
pb2 files can be regenerated by running: | ||
|
||
``` | ||
python3 -m grpc_tools.protoc -I../proto/ --python_out=gubernator/ --pyi_out=gubernator/ --grpc_python_out=gubernator/ --mypy_grpc_out=gubernator/ ../proto/*.proto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this stuff to a bash script in proto
directory? And include a pip install
for the tools.
And preferably we shouldn't need post-processing with sed
commands. Looks like it's a matter of getting CWD or input paths right to fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And preferably we shouldn't need post-processing with
sed
commands. Looks like it's a matter of getting CWD or input paths right to fix that.
Literally spent a couple of hours looking at this. In summary, it appears that Google have no interest in supporting modules which would require relative imports. It might be possible to get something to work by changing the proto directory, but I figured that might break things elsewhere. Feel free to have a go yourself though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you can probably tell, we don't use this Python functionality anymore. At least, not for Python 3. I'm thinking the entire Python client needs to be reimplemented from protos and disregard any consideration for legacy requirements.
I committed some changes to my branch for a genproto.sh
script that already exists but needed some cleaning up. It does generate the code for Go and Python ok. There's some old unit tests in python/tests
, but no idea how compatible that is to now current code. It fails, as you might expect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sed
calls are not related to legacy (in fact, it apparently worked on Python 2 without rewriting the imports). It is because Google apparently just dumps all (thousands) of their files in a top-level directory that is on their PYTHONPATH.
For an installable package, we can't expect users to change their PYTHONPATH to point to the package's source code (I know that sound ridiculous, but I literally don't understand why the protobuf maintainers in that thread don't appear to understand that). So, sed
rewrites the imports to import from the package (i.e. it could be done either as from gubernator
or from .
).
To test, run pip install .
in your python directory, then (ensuring you're not in the source directory) open python and import gubernator
. Without the sed
changes you'll get an import error.
I'll go through the other changes next week.
Is this good to merge? |
@Baliedge are we good with this PR? I'm about to break stuff, and it would be very nice to get this in before I do so. |
@thrawn01 @Dreamsorcerer I was unable to verify the python changes in a docker container using provided unit test. Would like to see an automated test in Github Actions, but that could be a future PR. Aside that, the merge conflicts need resolution. docker run --rm -it -v $(pwd):/src -w /src ubuntu
apt update
apt install -y gcc g++ python3 python3-dev python3-pip
pip install pytest
pip install -r python/requirements.txt
pytest python Relevant output:
Please fix test as necessary. |
Are you sure it's not using an old, previously installed, version of the code? That looks like one of the errors this PR fixes. Maybe try |
Also, the scripts have been deleted from the repository. How do you want to regenerate the files now? Shall I revert to the README? |
So sorry about that @Dreamsorcerer, the scripts were removed because we are using https://buf.build/ now. I've been quite distracted in the past few months, and the next few weeks leading up to Black Friday will probably be even more distracted. Let me know what you need, and If you have some time to work on this PR, please ping me in the PR, and I'll make sure it gets merged. |
@thrawn01 I can update the merge conflicts, but I don't know anything about buf. Does the Python build need to be added to buf somewhere? Or, should I just readd the build script for Python? |
Or readd the README from before, so the steps can be run manually? |
I can try to get |
Merged, should be ready again. |
Note this still won't work, as it doesn't have the change discussed above: #176 (comment) |
Sorry, I'm unable to give proper Python direction to support correct module structure. We don't use the Python client in our local usage of Gubernator, so I'm at the will of the public to provide direction there. Because the protobuf build script process changed to "Buf", could you please start a new PR and apply your recommended changes from there? |
It's literally just the Also, just noticed that the new one is missing the .pyi files for typing support. I added these with an extra couple of options to protoc: |
The module seems so out of date, it didn't seem like breaking backwards compatibility would be a big deal.
Let me know if anything needs to change.