KeyError when declaring an enumeration

[0x2b3bfa0]

Error

Traceback (most recent call last):
  File "/···/test/lib/python3.9/site-packages/google/protobuf/descriptor_database.py", line 129, in FindFileContainingSymbol
    return self._file_desc_protos_by_symbol[symbol]
KeyError: 'Genre'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/···/test/lib/python3.9/site-packages/google/protobuf/descriptor_database.py", line 138, in FindFileContainingSymbol
    return self._file_desc_protos_by_symbol[top_level]
KeyError: ''

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/···/test/lib/python3.9/site-packages/proto/enums.py", line 86, in __new__
    file_info.generate_file_pb(new_class=cls, fallback_salt=full_name)
  File "/···/test/lib/python3.9/site-packages/proto/_file_info.py", line 138, in generate_file_pb
    descriptor = pool.FindEnumTypeByName(full_name)
  File "/···/test/lib/python3.9/site-packages/google/protobuf/descriptor_pool.py", line 529, in FindEnumTypeByName
    self._FindFileContainingSymbolInDb(full_name)
  File "/···/test/lib/python3.9/site-packages/google/protobuf/descriptor_pool.py", line 723, in _FindFileContainingSymbolInDb
    raise error
  File "/···/test/lib/python3.9/site-packages/google/protobuf/descriptor_pool.py", line 718, in _FindFileContainingSymbolInDb
    file_proto = self._internal_db.FindFileContainingSymbol(symbol)
  File "/···/test/lib/python3.9/site-packages/google/protobuf/descriptor_database.py", line 141, in FindFileContainingSymbol
    raise KeyError(symbol)
KeyError: 'Genre'

Environment

• macOS 10.15.7
• Python 3.9.1

Steps to reproduce

Configure a virtual environment

virtualenv test

created virtual environment CPython3.9.1.final.0-64 in 577ms
  creator CPython3Posix(dest=/···/test, clear=False, no_vcs_ignore=False, global=False)
  seeder FromAppData(download=False, pip=bundle, setuptools=bundle, wheel=bundle, via=copy, app_data_dir=/···/virtualenv)
    added seed packages: pip==20.3.1, setuptools==51.0.0, wheel==0.36.1
  activators BashActivator,CShellActivator,FishActivator,PowerShellActivator,PythonActivator,XonshActivator

source ./$_/bin/activate

Install the package

pip install proto-plus

pip install proto-plus
Collecting proto-plus
  Using cached proto_plus-1.13.0-py3-none-any.whl
Collecting protobuf>=3.12.0
  Using cached protobuf-3.14.0-py2.py3-none-any.whl (173 kB)
Collecting six>=1.9
  Using cached six-1.15.0-py2.py3-none-any.whl (10 kB)
Installing collected packages: six, protobuf, proto-plus
Successfully installed proto-plus-1.13.0 protobuf-3.14.0 six-1.15.0

Run the official example

python

>>> import proto
>>> class Genre(proto.Enum):
...    GENRE_UNSPECIFIED = 0
...    CLASSICAL = 1
...    JAZZ = 2
...    ROCK = 3
...

Possible solutions

1. Placing the enumeration declaration inside a package and setting __protobuf__, as done in marshalling tests:

   proto-plus-python/tests/test_marshal_types_enum.py

   Line 21 in 50b87af

   __protobuf__ = proto.module(package="test.marshal.enum")

2. Defining enumeration names as a strings inside message fields prior to the actual enumeration declaration:

   proto-plus-python/tests/test_fields_enum.py

   Lines 19 to 27 in 50b87af

   	def test_outer_enum_init():
   	class Foo(proto.Message):
   	color = proto.Field(proto.ENUM, number=1, enum="Color")
   	class Color(proto.Enum):
   	COLOR_UNSPECIFIED = 0
   	RED = 1
   	GREEN = 2
   	BLUE = 3

[0x2b3bfa0]

My short-term solution has been adding this line to every affected source file:

__protobuf__ = proto.module(package=__name__)

If this behavior is intended, wouldn't be nice to default __protobuf__ to that value?

[software-dov]

Yes, when writing tests and defining proto-plus types inline in those tests, you've found the necessary workaround. An alternative would be to make the enum a nested type of the message type.

E.g.

def test_squid():
    class Squid(proto.Message):
        class Color(proto.Enum):
            COLOR_UNSPECIFIED = 0
            RED = 1
            GREEN = 2
            BLUE = 3
            BROWN = 4
        
        class Chromatophore(proto.Message):
            color = proto.Field(Color, number=1)
            
        chromatophores = proto.RepeatedField(Chromatophore, number=1)
        
    s = Squid()

This is an unfortunate but hopefully minor wart that's part of how proto plus defines and organizes manifests. The manifests are necessary because we need to make sure that all message and enum types have been defined before we send that data to the underlying protobuf implementation to generate types.
The package naming convention is intended to follow the protobuf package naming convention, since most proto-plus classes are expected to map to a corresponding entry in a .proto file.
Does any of that clarify the situation?

[0x2b3bfa0]

Thank you very much, @software-dov!

As per the style guide, it's «[preferible to prefix] enum values instead of surrounding them in an enclosing message», so that minor wart is good enough for me. Nevertheless, I think that it would be nice to, either:

1. Document the peculiarity and, perhaps, raise a more specific exception with a descriptive message when this happens.

2. Use a fallback module name to avoid this error, at the cost of using the __name__ attribute as the package name.

   proto-plus-python/proto/_package_info.py

   Line 39 in 6a43682

   package = getattr(proto_module, "package", "")

   diff --git a/proto/_package_info.py b/proto/_package_info.py
   index d01dd79..377e601 100644
   --- a/proto/_package_info.py
   +++ b/proto/_package_info.py
   @@ -36,7 +36,7 @@ def compile(name, attrs):
        proto_module = getattr(module, "__protobuf__", object())
   
        # A package should be present; get the marshal from there.
   -    package = getattr(proto_module, "package", "")
   +    package = getattr(proto_module, "package", module.__name__)
        marshal = Marshal(name=getattr(proto_module, "marshal", package))
   
        # Done; return the data.

   The proposal above passes all the tests, and does not seem to have any side effect. In my case, __name__ follows both naming conventions, and was the ideal choice, but this may not be the case in every circumstance. In fact, it can eventually have unusual values, like __main__ when running code from a top-level file or an interactive prompt.

[software-dov]

As per the style guide, it's _«[preferible to prefix] enum values instead of surrounding them in an enclosing message»

That's a very good point. I personally would argue that it's less important when writing tests, but on the other hand constant consistency with the style guide is no bad thing.

Whether or not it's a good idea to define proto types from a top-level file is a discussion for another day. I don't think that making this change is likely to break anything there.

Defining message/enum types from an interactive prompt already has a few unexpected side effects that complicate interactive development. I'm less concerned with breaking changes to this workflow since by definition it isn't part of a codebase.

Making the module name the default package name seems reasonable. I'll make a new bugfix tracking issue and reference this in that in order to preserve history without cluttering the description.

[0x2b3bfa0]

That's a very good point. I personally would argue that it's less important when writing tests, but on the other hand constant consistency with the style guide is no bad thing.

Agreed! In my particular case, it matters because of readability and deduplication, but it wouldn't otherwise.

Whether or not it's a good idea to define proto types from a top-level file is a discussion for another day.

I wouldn't mind doing so for a quick sanity check, but it doesn't look like one of the the best practices for any minimally stable piece of software; we can postpone that debate ad æternum.

I don't think that making this change is likely to break anything there. [...] Making the module name the default package name seems reasonable. I'll make a new bugfix tracking issue and reference this in that in order to preserve history without cluttering the description.

Great! I've opened #176 to fix #175.

Defining message/enum types from an interactive prompt already has a few unexpected side effects that complicate interactive development. I'm less concerned with breaking changes to this workflow since by definition it isn't part of a codebase.

Yes, it looks like it's impossible to redefine a protocol buffer class once it has been registered and there might be similar state-related issues for other interactive-specific behaviors. Nevertheless, solving them doesn't look like a priority right now.

[0x2b3bfa0]

Feel free to reopen this issue if you deem it necessary.