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

Make generated python files compatible with Cython #11011

Closed
wants to merge 1 commit into from

Conversation

vthib
Copy link
Contributor

@vthib vthib commented Nov 17, 2022

Replace access to variables created in globals() by explicit access to the globals() array. This avoids static analysis of the code to error out on those unknown variables, and make the files cythonable.

Fixes #10800

Replace access to variables created in globals() by explicit access to
the globals() array. This avoids static analysis of the code to error
out on those unknown variables, and make the files cythonable.
@vthib vthib requested a review from a team as a code owner November 17, 2022 13:16
@vthib vthib requested review from deannagarcia and removed request for a team November 17, 2022 13:16
copybara-service bot pushed a commit that referenced this pull request Dec 5, 2022
Replace access to variables created in globals() by explicit access to the globals() array. This avoids static analysis of the code to error out on those unknown variables, and make the files cythonable.

Fixes #10800

Closes #11011

FUTURE_COPYBARA_INTEGRATE_REVIEW=#11011 from vthib:10800-cython-compatibility 705da40
PiperOrigin-RevId: 493055381
@copybara-service copybara-service bot closed this in 9aa5272 Dec 6, 2022
@vthib vthib deleted the 10800-cython-compatibility branch December 12, 2022 17:59
@garysferrao
Copy link

@haberman Sorry, you referenced this PR last month. But it seems to have been closed by a bot. Do you have any plan to release this? I'm using flake8, and face a similar problem.

poetry run flake8 --version
4.0.1 (mccabe: 0.6.1, pycodestyle: 2.8.0, pyflakes: 2.4.0) CPython 3.9.16 on Linux

@deannagarcia
Copy link
Member

This got closed by a bot because it was submitted separately here: e3ea044. We use a tool that sometimes does this where it closes a PR and then submits the commit separately.

On a manual check, I'm pretty sure this got into the 22.x branch (e3ea044), so it should be in the 22.0/22.1 release. Are you using that version? If you still see this issue in 22, please open a new issue so we can investigate it separately.

@garysferrao
Copy link

@deannagarcia thank you for your reply. it works with v22 release!~ 😄

sorry it took so long to revert. i was unable to test it properly in my CI flow, mainly because it's not available in grpcio-tools v1.53.0 yet. i think i have to wait for it to be released here to properly list it in dependencies and CI tools.

₹ pip install grpcio-tools # v1.53.0 as of 2023-03-31
₹ python -m grpc_tools.protoc --version
libprotoc 3.21.12
₹ python -m grpc_tools.protoc -I=. --python_out=. --grpc_python_out=. --mypy_out=. ./xxx.proto
₹ python -m flake8 ./
xxx_pb2.py:nnn:5: F821 undefined name '_XXXREQUEST'
…

but i generated the *pb2.py file with v22.2 from the release page.

₹ wget https://github.com/protocolbuffers/protobuf/releases/download/v22.2/protoc-22.2-linux-x86_64.zip -o ~/protoc/protoc-22.2-linux-x86_64.zip
₹ unzip ~/protoc/protoc-22.2-linux-x86_64.zip

₹ ~/protoc/bin/protoc --version
libprotoc 22.2
₹ ~/protoc/bin/protoc -I=. --python_out=. ./*.proto
₹ python -m flake8 ./
# no error related to global variables

thanks for your help. 🙇

vthib added a commit to vthib/protobuf that referenced this pull request Aug 18, 2023
This is a follow-up to
protocolbuffers#11011

The generation is still not compatible with Cython when maps are used.

For example, this protobuf file:

```
syntax = "proto3";

message Foo {
    map<string, string> bar = 1;
}
```

Will generate:

```
...
_globals = globals()
_builder.BuildMessageAndEnumDescriptors(DESCRIPTOR, _globals)
_builder.BuildTopDescriptorsAndMessages(DESCRIPTOR, 'a_pb2', _globals)
if _descriptor._USE_C_DESCRIPTORS == False:

  DESCRIPTOR._options = None
  _FOO_BARENTRY._options = None
  _FOO_BARENTRY._serialized_options = b'8\001'
  _globals['_FOO']._serialized_start=11
  _globals['_FOO']._serialized_end=88
  _globals['_FOO_BARENTRY']._serialized_start=46
  _globals['_FOO_BARENTRY']._serialized_end=88
```

The `_FOO_BARENTRY` variable is not defined anywhere and confuses
cython. We can see the `_globals` used below, it is simply missing for
the first two lines using `_FOO_BARENTRY`.
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.

Python generated code is no longer compatible with Cython since 3.20
5 participants