Skip to content

fix python code generation compatibility with Cython#13593

Closed
vthib wants to merge 1 commit intoprotocolbuffers:mainfrom
vthib:fix-cython-compatibility
Closed

fix python code generation compatibility with Cython#13593
vthib wants to merge 1 commit intoprotocolbuffers:mainfrom
vthib:fix-cython-compatibility

Conversation

@vthib
Copy link
Copy Markdown
Contributor

@vthib vthib commented Aug 18, 2023 •
edited
Loading

This is a follow-up to #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.

If possible, i would love to have this fix in the next 24 release, instead of having to wait for the 25 release.

@vthib
fix python code generation compatibility with Cython
8c37e01 
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`.
@vthib vthib requested a review from a team as a code owner August 18, 2023 09:59
@vthib vthib requested review from anandolee and removed request for a team August 18, 2023 09:59
@anandolee
Copy link
Copy Markdown
Contributor

anandolee commented Aug 28, 2023

Thank you for the PR. As it changes generated code, I may need to run a wide tests in google internal and submit there

@copybara-service copybara-service Bot mentioned this pull request Aug 28, 2023
Closed
copybara-service Bot pushed a commit that referenced this pull request Aug 28, 2023
@anandolee @copybara-github
fix python code generation compatibility with Cython
bf519a9 
copied from #13593

PiperOrigin-RevId: 560774287
@anandolee anandolee requested review from deannagarcia and removed request for deannagarcia August 28, 2023 18:49
@anandolee anandolee self-assigned this Aug 28, 2023
@anandolee
Copy link
Copy Markdown
Contributor

anandolee commented Aug 28, 2023 •
edited
Loading

Turns out tests can not pass. Example error:

_globals['_ANY.fields_by_name['type_url']']._options = None
SyntaxError: invalid syntax. Perhaps you forgot a comma?

I will update the fix

copybara-service Bot pushed a commit that referenced this pull request Aug 28, 2023
@anandolee @copybara-github
fix python code generation compatibility with Cython
5c2264c 
raised by #13593

PiperOrigin-RevId: 560774287
copybara-service Bot pushed a commit that referenced this pull request Aug 29, 2023
@anandolee @copybara-github
fix python code generation compatibility with Cython
e65d051 
raised by #13593

PiperOrigin-RevId: 560774287
copybara-service Bot pushed a commit that referenced this pull request Aug 29, 2023
@anandolee @copybara-github
fix python code generation compatibility with Cython
20faf56 
raised by #13593

PiperOrigin-RevId: 561077681
@anandolee
Copy link
Copy Markdown
Contributor

anandolee commented Aug 29, 2023

should be fixed with e65d051

@anandolee anandolee closed this Aug 29, 2023
@vthib vthib deleted the fix-cython-compatibility branch August 29, 2023 18:30
@vthib
Copy link
Copy Markdown
Contributor Author

vthib commented Aug 29, 2023

Thanks!

@vthib
Copy link
Copy Markdown
Contributor Author

vthib commented Aug 30, 2023

@anandolee Is it possible to backport this fix into the next 24.X release?

@vthib
Copy link
Copy Markdown
Contributor Author

vthib commented Sep 8, 2023

@anandolee Sorry for the bother, I saw the new release 24.3 release but it does not contain this fix. Is it possible to backport it for the next 24.X release please?

@vthib
Copy link
Copy Markdown
Contributor Author

vthib commented Sep 21, 2023

@anandolee Can I do a PR to backport this fix into the 24.X branch?

@vthib vthib mentioned this pull request Sep 28, 2023
Merged
@vthib
Copy link
Copy Markdown
Contributor Author

vthib commented Sep 28, 2023

I pushed a PR request for the backport here: #14240

vthib pushed a commit to vthib/protobuf that referenced this pull request Sep 28, 2023
@anandolee @vthib
fix python code generation compatibility with Cython
fe87c90 
raised by protocolbuffers#13593

PiperOrigin-RevId: 561077681
@hothanhloc68
Copy link
Copy Markdown

hothanhloc68 commented Oct 12, 2023

This is a follow-up to #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.

If possible, i would love to have this fix in the next 24 release, instead of having to wait for the 25 release.

@hothanhloc68
Copy link
Copy Markdown

hothanhloc68 commented Oct 12, 2023

This is a follow-up to #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.

If possible, i would love to have this fix in the next 24 release, instead of having to wait for the 25 release.

@hothanhloc68
Copy link
Copy Markdown

hothanhloc68 commented Oct 12, 2023

Turns out tests can not pass. Example error:

_globals['_ANY.fields_by_name['type_url']']._options = None
SyntaxError: invalid syntax. Perhaps you forgot a comma?

I will update the fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anandolee anandolee Awaiting requested review from anandolee anandolee is a code owner automatically assigned from protocolbuffers/protobuf-python

1 more reviewer

@hothanhloc68 hothanhloc68 hothanhloc68 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@anandolee anandolee

Labels

python

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vthib @anandolee @hothanhloc68