Mutating '__module__' of proto-generated messages considered harmful

[tseaver]

From #4695 (comment):

I wrote:

I still cannot see how it is correct to scribble on the message objects defined outside spanner: it hides the actual source location, and worse, is likely to get stomped on by every one of our modules which uses it.

E.g.:

>>> from google.longrunning import operations_pb2
>>> operations_pb2.CancelOperationRequest.__module__
'google.longrunning.operations_pb2'
>>> import google.cloud.spanner_admin_instance_v1.types
>>> operations_pb2.CancelOperationRequest.__module__
'google.cloud.spanner_admin_instance_v1.types'
>>> import google.cloud.spanner_admin_database_v1.types
>>> operations_pb2.CancelOperationRequest.__module__
'google.cloud.spanner_admin_database_v1.types'
>>> import google.cloud.language_v1beta2.types
>>> operations_pb2.CancelOperationRequest.__module__
'google.cloud.language_v1beta2.types'

@dhermes responded:

I am with @tseaver here.

@lukesneeringer responded:

I agree that this is a problem in theory. I am less convinced that it is a problem in practice.

The key problem that this solves is with documentation: There is no good way (as far as I can tell) to tell Sphinx to present the message objects at a sane location other than doing what I did.

I am all for an alternative method that gets a similar result. Essentially what I want is:

• The message classes must be documented, and presented as part of the overall package.
• The documentation should not make the user think about _pb2 junk.
• Users should not have to think about the internal proto sorting.

Basically, if there is some good way to have Sphinx present the modules in the location where we want to teach users to import them from, then I am completely fine with doing that in favor of what I have now.

(I am also fine with being told that my goals above should be non-goals, with supporting argumentation of course.)

I replied:

Until we are generating Sphinx docs from post-repo-split world, given the "last one wins" import race I demonstrated above, I don't believe your current solution actually means what you think it means. The messages will be generated with whatever module happened to be imported last as their faux location.

The documentation should not make the user think about _pb2 junk.

I would argue that this goal is the source of the breakage, and should be removed. For instance, users who look at docs for stdlib modules are not confused by references to top-level imports from other modules. Knowing that the message comes from proto-generated code (and which proto to examine, if need be) is actually claritying, IMNSHO.

Even if we agreed that hiding the _pb2 was a good thing for messages defined inside e.g., google.cloud.spanner, it cannot be the Right Thing (TM) to pretend that stuff like LRO, Empty, etc. are defined there.

[theacodes]

We can likely resolve this using a sphinx plugin. :)

[tseaver]

I'm still dubious about hiding the actual source module for "local" messages, but that ship is likely sailed.