danielgtaylor / python-betterproto

Clean, modern, Python 3.6+ code generator & library for Protobuf 3 and async gRPC
MIT License
1.51k stars 214 forks source link

ImportError: cannot import name for ellipsis imports #178

Open hosaka opened 3 years ago

hosaka commented 3 years ago

Setup:

❯ tree                                                                                                                                               
.
├── Content.proto
├── Detection.proto
└── lib

1 directory, 2 files

Content.proto

syntax = "proto3";

package My.Content;

import "Detection.proto";

message Content {
  enum Type {
    TYPE_UNSPECIFIED = 0;
    TYPE_VIDEO = 1;
  }
  Type type = 1;
  My.Detection.Detection.Gender gender = 2;
}

Detection.proto

syntax = "proto3";

package My.Detection;

message Detection {
  enum Gender {
    GENDER_UNSPECIFIED = 0;
    GENDER_FEMALE = 1;
    GENDER_MALE = 2;
  }
  Gender gender = 1;
}

protoc:

❯ protoc -I . --python_betterproto_out=lib *.proto 
❯ tail -n 2 lib/My/Content/__init__.py                                                                                                               
from ... import MyContentContentType as __MyContentContentType__
from ... import MyDetectionDetectionGender as __MyDetectionDetectionGender__

Test:

❯ python3                                                                                                                                      265ms 
Python 3.6.9 (default, Oct  8 2020, 12:12:24) 
[GCC 8.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from lib.My import Content
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/wa/qt/better/lib/My/Content/__init__.py", line 23, in <module>
    from ... import MyContentContentType as __MyContentContentType__
ImportError: cannot import name 'MyContentContentType'
>>> 

This is a small example, I get a different error in my actual project, but I suspect it's related: ValueError: attempted relative import beyond top-level package. Shouldn't the import read something along the lines of:

from ..Content import ContentType as __MyContentContentType__
from ..Detection import DetectionGender as __MyDetectionDetectionGender__

Is this a know issue or am I doing something out of the ordinary? I am using the same proto files to generate C++ code.

leenr commented 3 years ago

Got the same problem.

The package name for proto files I'm trying to use starts with capital letter (like in an example in this issue), but it seems that betterproto does not support capital letters in the first component of the package name (regex inside parse_source_type_name is failing).

I've replaced regex with r"^\.?(.+)\.(.+)" and it has solved this problem.

Is it intentional?

leenr commented 3 years ago

I'm sorry, I didn't do enough research before writing previous comment :( As I understand now, that issue is due to the way betterproto trying to find where the package name ends: it relies on Google's Protocol Buffers Style Guide that package name are in lowercase and message names are capitalized (though, it "should" be that way, not "must").

Unfortunately, I do not have control over mentioned .proto files, so that issue for me will always require manual code editing after compiling :( Is there a plan to implement a better way to split type names? I'm ready to help in my spare time, if it will be okay for betterproto to use different solution, e.g. collect all package names before compiling.

Gobot1234 commented 3 years ago

I think it should be possible you support it using pathlib and checking if it's a directory or a file

leenr commented 3 years ago

I think it should be possible you support it using pathlib and checking if it's a directory or a file

No, as I understand, this solution will not work. Internal representations (incl. decision which imports should be written) is generated before any files has been written to filesystem. Also, files are written by protoc based on response from betterproto, not by the betterproto itself. And, on top of it, there may be a problem with recursive references.

For now, I've implemented a quick fix for me based on output_file.parent_request.output_packages.keys(): https://gist.github.com/leenr/3cf377fad2600dcddb419e9bdca28b92. It appears to be working, though, I cannot say for sure that it is architectural ideal. I plan to do more research later, after I'll pinpoint another bug in betterproto.

ziima commented 3 years ago

I reproduced the bug, but it seems to be only issue if --python_betterproto_out=..

hosaka commented 3 years ago

@ziima I am using --python_betterproto_out=lib and get the same errors as described in the first post. The protoc is invoked in setup.py in my case, as a workaround I simply added a step which uses regex to fix the imports, which is far from perfect. The problem is still preset.

Jasha10 commented 3 years ago

Here is a small minimal repro:

$ ls
MyPackage.proto
$ cat MyPackage.proto
syntax = "proto3";
package MY_PACKAGE;
enum EncodingEnum
{
  MEMBER_ZERO = 0;
  MEMBER_ONE = 1;
}
message MyMessage
{
        int32 MessageVersion = 1;
        EncodingEnum Encoding = 2;
}
$ mkdir lib
$ protoc --python_betterproto_out=lib MyPackage.proto
Writing MY_PACKAGE/__init__.py
Writing __init__.py
$ cat lib/__init__.py  # lib/__init__.py is empty!
$ cat lib/MY_PACKAGE/__init__.py
# Generated by the protocol buffer compiler.  DO NOT EDIT!
# sources: MyPackage.proto
# plugin: python-betterproto
from dataclasses import dataclass

import betterproto
from betterproto.grpc.grpclib_server import ServiceBase

class EncodingEnum(betterproto.Enum):
    MEMBER_ZERO = 0
    MEMBER_ONE = 1

@dataclass(eq=False, repr=False)
class MyMessage(betterproto.Message):
    message_version: int = betterproto.int32_field(1)
    encoding: "_MyPackageEncodingEnum__" = betterproto.enum_field(2)

from .. import MyPackageEncodingEnum as _MyPackageEncodingEnum__

And as a workaround, you can replace the final line of lib/MY_PACKAGE/__init__.py with this:

_MyPackageEncodingEnum__ = EncodingEnum

Making this replacement solves the ImportError.