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: Fixed the compatibility with conda, protobuf 4 and pyext protobuf #471

Closed
wants to merge 2 commits into from

Conversation

Ark-kun
Copy link

@Ark-kun Ark-kun commented Jul 9, 2024

@Ark-kun Ark-kun requested a review from a team as a code owner July 9, 2024 04:21
Copy link

conventional-commit-lint-gcf bot commented Jul 9, 2024

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

Copy link
Contributor

@parthea parthea left a comment

Choose a reason for hiding this comment

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

Is it possible to add a test that fails? I'm not able to reproduce the issue.

@happy-qiao
Copy link

happy-qiao commented Jul 9, 2024

Could you elaborate how this change can fix pyext issue? Essentially, the PR doesn't seem to change the value of exported variables: "repeated_composite_types", "repeated_scalar_types", "map_composite_types".

Is it for the case when upb and pyext both exists and the change makes it default to pyext? But pyext is deprecated, if default to pyext, will it raise other issues?

7hmJ6Rkgc2spo4n (1)

@parthea parthea marked this pull request as draft July 9, 2024 18:31
@Ark-kun
Copy link
Author

Ark-kun commented Jul 10, 2024

@parthea

Is it possible to add a test that fails?

Do you have Conda tests environments?

I'm not able to reproduce the issue.

I think you've added repro steps to the internal doc.

Also there are repro steps in the linked issue: #470

@Ark-kun
Copy link
Author

Ark-kun commented Jul 10, 2024

Could you elaborate how this change can fix pyext issue? Essentially, the PR doesn't seem to change the value of exported variables: "repeated_composite_types", "repeated_scalar_types", "map_composite_types".

Let's carefully examine the execution flow. The code does change the items of the lists and the number of those items.

Is it for the case when upb and pyext both exists

Yes. This is the exact case where you and our customers have issues.

>>> from google._upb import _message as msg1
>>> msg1.RepeatedCompositeContainer
<class 'google._upb._message.RepeatedCompositeContainer'>

>>> from google.protobuf.pyext import _message as msg2
>>> msg2.RepeatedCompositeContainer
<class 'google.protobuf.pyext._message.RepeatedCompositeContainer'>

The root cause of this proto-plus bug is that proto-plus does not recognize pyext types when ubp types exist. But conda's protobuf defaults to pyext despite ubp existing.

and the change makes it default to pyext?

No. The change does not change the default. It adds the pyext types to the lists.

But pyext is deprecated

pyext is used by real customers who want to use Vertex AI. pyext is used by default in protobuf installed by conda.

@Ark-kun Ark-kun marked this pull request as ready for review July 10, 2024 02:41
@parthea parthea assigned parthea and unassigned Ark-kun Jul 11, 2024
@parthea parthea added the status: investigating The issue is under investigation, which is determined to be non-trivial. label Jul 11, 2024
@parthea parthea added the owlbot:run Add this label to trigger the Owlbot post processor. label Jul 12, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jul 12, 2024
@parthea parthea assigned Ark-kun and unassigned parthea Jul 12, 2024
Copy link
Contributor

@parthea parthea left a comment

Choose a reason for hiding this comment

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

Please could you add a comment in the code which explains the reason that we need to include both upb and pyext types? You mentioned this in the PR comments but it should be included in code comments as well.

Please could you also fix the failing presubmits ? This PR needs to be updated to cater for #474 which was merged

Otherwise, LGTM

@parthea parthea marked this pull request as draft July 12, 2024 16:28
@parthea parthea removed the status: investigating The issue is under investigation, which is determined to be non-trivial. label Jul 12, 2024
Copy link
Contributor

@parthea parthea left a comment

Choose a reason for hiding this comment

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

Would you be able to refactor the code to reduce duplication? For example, can we use a helper function instead of having repeated code?

Comment on lines +53 to +56
try:
from google.protobuf.pyext import _message as _message_pyext
except ImportError:
_message_pyext = None
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this import statement to the top of the file since we're importing it as _message_pyext

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants