Using Pydantic creates circular imports

AdrienVannson commented 4 weeks ago

Summary

Using Pydantic creates circular imports

Reproduction Steps

Compile the following .proto files:

A_a.proto:

syntax = "proto3";
package root.apackage;

import "B.proto";

message AaMsg {
    root.bpackage.BMsg x = 1;
}

A_b.proto:

syntax = "proto3";
package root.apackage;

message AbMsg {
    int32 x = 1;
}

B.proto:

syntax = "proto3";
package root.bpackage;

import "A_b.proto";

message BMsg {
    apackage.AbMsg x = 1;
}

There is no circular dependency between the .proto files. However, after the compilation, there is a circular dependency between the two packages.

Expected Results

I compiled the .proto files with the pydantic_dataclasses. I would expect importing the library with import root.apackage to work correctly.

Actual Results

import root.apackage fails with the following error:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "XXX/root/apackage/__init__.py", line 17, in <module>
    from .. import bpackage as _bpackage__
  File "XXX/root/bpackage/__init__.py", line 21, in <module>
    class BMsg(betterproto.Message):
  File "XXX/.venv/lib/python3.10/site-packages/pydantic/dataclasses.py", line 249, in create_dataclass
    pydantic_complete = _pydantic_dataclasses.complete_dataclass(
  File "XXX/.venv/lib/python3.10/site-packages/pydantic/_internal/_dataclasses.py", line 120, in complete_dataclass
    set_dataclass_fields(cls, types_namespace, config_wrapper=config_wrapper)
  File "XXX/.venv/lib/python3.10/site-packages/pydantic/_internal/_dataclasses.py", line 82, in set_dataclass_fields
    fields = collect_dataclass_fields(cls, types_namespace, typevars_map=typevars_map, config_wrapper=config_wrapper)
  File "XXX/.venv/lib/python3.10/site-packages/pydantic/_internal/_fields.py", line 313, in collect_dataclass_fields
    ann_type = _typing_extra.eval_type_lenient(dataclass_field.type, types_namespace, cls_localns)
  File "XXX/.venv/lib/python3.10/site-packages/pydantic/_internal/_typing_extra.py", line 240, in eval_type_lenient
    return eval_type_backport(value, globalns, localns)
  File "XXX/.venv/lib/python3.10/site-packages/pydantic/_internal/_typing_extra.py", line 264, in eval_type_backport
    return typing._eval_type(  # type: ignore
  File "/usr/lib/python3.10/typing.py", line 327, in _eval_type
    return t._evaluate(globalns, localns, recursive_guard)
  File "/usr/lib/python3.10/typing.py", line 694, in _evaluate
    eval(self.__forward_code__, globalns, localns),
  File "<string>", line 1, in <module>
AttributeError: partially initialized module 'pygrpc2_issue.root.apackage' has no attribute 'AbMsg' (most likely due to a circular import)

System Information

libprotoc 3.12.4
Python 3.10.12
Name: betterproto
Version: 2.0.0b6
Summary: A better Protobuf / gRPC generator & library
Home-page: https://github.com/danielgtaylor/python-betterproto
Author: Daniel G. Taylor
Author-email: danielgtaylor@gmail.com
License: MIT
Location: XXX/.venv/lib/python3.10/site-packages
Requires: grpclib, python-dateutil, typing-extensions
Required-by: pygrpc-poc

Checklist

[X] I have searched the issues for duplicates.
[X] I have shown the entire traceback, if possible.
[X] I have verified this issue occurs on the latest prelease of betterproto which can be installed using pip install -U --pre betterproto, if possible.

Notes

When Pydantic is not used, the circular dependency is handled correctly by Python.

AdrienVannson commented 3 weeks ago

I think that the problem can be solved by moving some imports to the end of the generated files, just before all the rebuild_dataclass. Before I try to do it and see if it actually work as expected, I have a question: is it something that we actually want to do?

Or should we consider that circular package dependencies are not acceptable and should indeed produce an error? As far as I know, they work correctly with the original gRPC python client and betterproto without pydantic, but I'm not sure about the other clients.

Gobot1234 commented 3 weeks ago

There's no reason for them not to work. Please if you do fix this add a test so this doesn't happen in the future.

AdrienVannson commented 3 weeks ago

Great, thanks! Actually, it seems that there is already a test for that: tests/inputs/import_circular_dependency. However, this test is only executed without pydantic, so no error is shown.

If I replace plugin_output_package = "tests.output_betterproto" by plugin_output_package = "tests.output_betterproto_pydantic" in test_inputs.py, I have the expected error... but a lot of other unrelated tests fail as well.

AdrienVannson commented 3 weeks ago

Actually, I have the impression that it is not that simple to fix. I tried putting the imports from get_type_reference at the end of the file, as I said. However, in the test import_circular_dependency, in the line of the generated file: other: "other.OtherPackageMessage" = betterproto.message_field(2), I am not sure why, but there seem to be a problem due to the fact that the name other is used twice.

After I fixed that quickly by adding a prefix, in my project, I still had errors occurring during the calls to rebuild_dataclass. As I'm not sure how long it would take me to make everything work and how many changes to the project will be needed, I think I'll stop here for now

AdrienVannson commented 1 week ago

In the end, I probably have a solution. I will test it, and if it works, I will send a PR once the other issues that I opened are fixed (I need the fix to solve this problem)

danielgtaylor / python-betterproto