fix: lookup attribute instead of performing a deepcopy

[GabrieleMazzola]

Closes #224.

[google-cla]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (main@3148a1c). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head dec4d6f differs from pull request most recent head 9cc0df0. Consider uploading reports for the commit 9cc0df0 to get more accurate results

@@           Coverage Diff            @@
##             main      #226   +/-   ##
========================================
  Coverage        ?   100.00%           
========================================
  Files           ?        22           
  Lines           ?      1004           
  Branches        ?       227           
========================================
  Hits            ?      1004           
  Misses          ?         0           
  Partials        ?         0           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3148a1c...9cc0df0. Read the comment docs.

[GabrieleMazzola]

@googlebot I signed it!

[software-dov]

It looks like the cpp runtime is causing problems because the type it exposes doesn't have the same python-visible fields. There's a couple of ways we can try solving this problem:

1. Complain to the protobuf folks and get a generic, runtime independent mechanism for determining the type of the elements of a repeated field.
2. Have a fast path (proposed change) for the python runtime and try/except to the deepcopy and canary.
3. Continue to dig deep for a field/mechanism that's faster than deep copy and works on the cpp runtime types.
4. Something like this (also passes the tests, but I'm not entirely sure it's 100% safe):

--- a/proto/marshal/marshal.py
+++ b/proto/marshal/marshal.py
@@ -155,12 +155,12 @@ class BaseMarshal:
         # Return a view around it that implements MutableSequence.
         value_type = type(value)  # Minor performance boost over isinstance
         if value_type in compat.repeated_composite_types:
-            return RepeatedComposite(value, marshal=self)
+            return RepeatedComposite(value, marshal=self, proto_type=proto_type)
         if value_type in compat.repeated_scalar_types:
             if isinstance(proto_type, type):
                 return RepeatedComposite(value, marshal=self, proto_type=proto_type)
             else:
-                return Repeated(value, marshal=self)
+                return Repeated(value, marshal=self, proto_type=proto_type)

[busunkim96]

How about we go with 2 to unblock folks, and follow up with the protobuf folks to figure out which of 1, 3, or 4 is the best option? Is there a person we can tag in to this PR?

I am somewhat uncomfortable with relying on a private attribute from protobuf, but it seems OK for the short term with a fallback (option 2).

[crwilcox]

Also +1 to option 2.

[craiglabenz]

Do we have a sense of what percentage of libraries would benefit from this change, versus trip the try/catch and actually run slower? I certainly don't, but I do know that python-datastore raises an AttributeError and would be further slowed.

As I'm thinking about this further, I suppose the answer is to only bump proto-plus versions in libraries that have been confirmed to work smoothly with this change?

I also don't know anything about the cpp layer and would love to learn more.

[busunkim96]

@craiglabenz The cpp option is opt-in according to the documentation, although I saw some comments on issues that seemed to suggest it was default.

https://developers.google.com/protocol-buffers/docs/reference/python-generated#cpp_impl

I think it would be helpful to clarify with a protobuf person who knows for sure.

[crwilcox]

We could also check for the attr without a try/catch if we are concerned about the cost of that.

[craiglabenz]

I am increasingly suspicious that this deepcopy is not the guilty party. In testing the implications of this change and the snippet proposed in the comment above, I just discovered that all of python-datastore's slowness must be coming from elsewhere, because my scratch code that recreates said slowness never reaches that deepcopy.

[software-dov]

Responding to a bunch of things all at once.

Do we have a sense of what percentage of libraries would benefit from this change, versus trip the try/catch and actually run slower?

There's a couple misconceptions in this question. The python/cpp protobuf runtimes are agnostic of the client libraries themselves: they are determined by platform and environment variables. The cpp runtime is much faster in certain real world workloads, but I don't have a good idea of what the breakdown in user environments between the cpp and py runtimes is.
Also, in most cases, try/catch is fast; especially if the straightforward path is more common, it is often faster to handle the common case in try and handle the uncommon in except than to do normal if else chains.

I saw some comments on issues that seemed to suggest [cpp] was default.

This is a little bit complicated. If a cpp runtime for a particular platform exists and is installed, it is now the default version. We can see this via code archaeology in api_implementation.py:40.

I am increasingly suspicious that this deepcopy is not the guilty party.

That's entirely possible. Designing a good, helpful benchmark that mimics real world uses is tricky. I gave up after about two hours trying to recreate the benchmark linked in the issue for this PR. On the other hand, RepeatedField serialization is a known real-world hotspot and have not been highly optimized. Trying to resolve this issue seems like a good idea, especially if it can be done without to much effort or stress.

[software-dov]

What are people's required timelines for dealing with this performance regression? Clearly sooner is better, but is anything being blocked, or has conversion to the new client libraries been halted, or anything?

[craiglabenz]

I ported this change to googleapis/python-datastore and discovered that these attributes are not universally available.

We might need to do something like this, but then assuming an equivalent change, this would LGTM.

if hasattr(self.pb, '_message_descriptor') and hasattr(self.pb._message_descriptor, '_concrete_class'):
    return self.pb._message_descriptor._concrete_class

canary = copy.deepcopy(self.pb).add()
return type(canary)

[software-dov]

these attributes are not universally available.

This is the difference between the cpp and python protobuf runtimes. The concrete type of self.pb is different depending on which runtime is being used; it is not dependent on the API or the client library itself.

The _message_descriptor attribute is apparently considered an implementation detail of the python based protobuf runtime.

[craiglabenz]

I partially followed that (due to my ongoing ignorance of the cpp layer), but to clarify, are you suggesting that this line of code will work everywhere?

[software-dov]

The change will work for all client libraries IFF the application process is using the python protobuf runtime.

This is the relevant chunk of the tech stack:

• User application
• Client library manual layer (optional. Firestore has a manual layer, Texttospeech does not)
• Generated client library, aka GAPIC
• Proto plus runtime, i.e. this repo
• General protobuf runtime, either written in pure python or in cpp as a python extension, which is determined at runtime. Proto plus should be agnostic about which which is being used.

The lowest layer in the above is library is preventing the general fix from merging. The two different implementations provide different unofficial APIs.

[crwilcox]

@software-dov can you expand on the pure python vs cpp protobuf runtime? How would I get to each of these? Which one is used by cloud libraries?

[software-dov]

Which one is used by cloud libraries?

It is chosen dynamically at runtime based on the platform (linux, macos X amd64, aarch64), version of protobuf installed, and environment variables. To be strictly general and not break cloud, any solution must be compatible with both. I would imagine, based on the environment they're running in, that most user applications tend to use the cpp runtime.

The protobuf runtime is responsible for memory layout, serialization and deserialization, and message introspection. It is the code that allocates memory and performs host-to-network and network-to-host bit conversion.

[crwilcox]

@software-dov I know of at least one instance where a user has reverted to the previous major version due to this regression relative to the monolith generator.

[busunkim96]

I opened https://groups.google.com/g/protobuf/c/pYcq-UBixqU to get for some clarification from the Protobuf folks on the specific situations protobuf defaults to the C++ implementation (as well as when the change was made).

If you're curious, you can figure out which implementation protobuf is using with this snippet. I get 'cpp' on a fresh install on my linux workstation.

from google.protobuf.internal import api_implementation
print(api_implementation.Type())

Note that protobuf discourages using api_implementation.Type() (comment). That leads me to think think they'd be open to adding a way to fetch the type of elements in a repeated field agnostic of the implementation type. I'll open another issue to start that discussion.

Are folks alright with the code in this PR coupled with a fallback to deepcopy to handle the CPP case (try/except, if/else, attribute checking)? It sounds like it made a significant difference for the PR author in a real world scenario. I imagine it will be a net positive for some folks and wouldn't make things worse for anyone else.

[stefanbluegrid]

Hello guys, what is happening with this PR? If it can be fixed with simple try/except or if statement, I don't see reason for not being merged.

[parthea]

Waiting for #312