Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python: support for optional python_package (or similar) to make import explicit #7061

Open
brocaar opened this issue Jan 6, 2020 · 19 comments

Comments

@brocaar
Copy link

brocaar commented Jan 6, 2020

What language does this apply to?

Python.

Describe the problem you are trying to solve.

The Python code generator uses the directory structure of the .proto files to generate the generated Python code directory layout and imports (e.g. when Protobuf file A imports Protobuf file B). It doesn't sanitize Python reserved keywords.

This can be problematic in two cases:

  1. The directory structure contains a reserved keyword. For example when it contains as. In python import as fails but also import as.foo.bar is invalid.

  2. When in case of A depending on B, A will contain an absolute import that might not be on the Python path (e.g. when the generated code is packaged so instead of import b it should do import mypackage.b). This was mentioned as a fix in the discussions: python: use relative imports in generated modules #1491, but I would consider this a workaround.

Describe the solution you'd like

For several languages, there are ways to make the package / module name explicit by defining an option, e.g. option go_package, option java_package, ... (https://developers.google.com/protocol-buffers/docs/proto3#packages). (I believe there is also a ruby_package #4627).

I believe an optional option python_package could solve the above issues as a as/api.proto file then could contain an option python_package = "as_pb.api" (or "my_package.as_pb.api").

The Protobuf code generator will then take the python_package into account when:

  • Generating the directory structure for the generated Python code so that it can be imported as import as_pb.api (as_pb/api.py).
  • When defining imports (Protobuf file A depends on B). The generated file A then imports B using the (optional) defined python_package.

Describe alternatives you've considered

My initial approach was to re-define the structure of the original Protobuf files. However, when the Protobuf files are used to generate other languages bindings, this has side-effects (e.g. the Go code now ends up in as_pb/api, it should be in as/api.

I believe the supported languages should not be leading in how the Protobuf structure looks like.

Additional context

Explicit is better than implicit.

(https://www.python.org/dev/peps/pep-0020/)

😉

See also brocaar/chirpstack-api#5 .

@tjerkw
Copy link

tjerkw commented Jan 17, 2020

I this this is a duplicate of #2283 I also really need it. For now i think we have todo rewriting of the imports after the code has been generated.

@prateek2408
Copy link

I face the same issue where the compiled proto files generated have the wrong import statements.

@djeer
Copy link

djeer commented Mar 5, 2020

Very old problem and no fix expected

@gitpushdashf
Copy link

I'm also interested in this.

@hungrybirder
Copy link

Hi, Any progress on this issue?

@stefanondisponibile
Copy link

same problem here. 🤦‍♂️

@vavsab
Copy link

vavsab commented Jan 6, 2021

same problem

@rmurthy716
Copy link

We also have the same problem...

@daicoden
Copy link

daicoden commented Jun 9, 2021

Is there some automated viable workaround? It seems like a necessity to conform to language guidelines and restrictions.

Instead of renaming hyphen to underscore automagically, I would vote it be explicit. I'm seeing it break down with other tools, i.e. bazel, and am having to fork and rename packages.

@stefanondisponibile
Copy link

@daicoden at the moment the only "automated" workaround I'm using is some bash vodoo to edit what I need after compiling the protobufs. Something along this:

sed -i 's/^import \(.\+\) as/from . import \1 as/' some/pkg/protobufs/*.py

@ashokpant
Copy link

ashokpant commented Jul 13, 2021

This code works for me for relative imports in the generated package
Create a file python2to3.py and add following code in it. Then run the script from the generated proto project root.

import os
path = os.path.dirname(os.path.dirname(os.path.realpath(__file__)))
path = os.path.join(path, 'src') # package name where generated files are reside
file_path = f'{path}/*_pb2*.py*'
os.system(f'2to3 -wn -f import {file_path}')

@elharo elharo added the python label Aug 21, 2021
@elharo elharo changed the title Python: support for optional python_package (or simmilar) to make import explicit Python: support for optional python_package (or similar) to make import explicit Sep 13, 2021
@alexprengere
Copy link

Another solution could be to have an option to generate all Python code in a single module, as this would solve the "absolute imports between modules" issue, that I personally solve using the sed trick mentioned above.

@ninebit
Copy link

ninebit commented May 12, 2022

Likewise, this is a challenge. Without firm convention and support here, interoperability within our greatly multi-lingual system is strained by competing conventions and workarounds. Editing the file that boldly says "Do Not Edit" asks for trouble.

rojas-diego added a commit to noted-eip/protorepo that referenced this issue Jul 6, 2022
* We're experiencing issues related to Python import paths in the generated protobuf and grpc code. See protocolbuffers/protobuf#7061

Co-authored-by: edouard-sn <58398928+edouard-sn@users.noreply.github.com>
Signed-off-by: Diego ROJAS <rojasdiegopro@gmail.com>
@aucampia
Copy link
Contributor

I'm trying to work with https://github.com/cloudevents/spec/blob/3da5643ebceb39637406a7e30903dbac81cf92d2/cloudevents/formats/cloudevents.proto

package io.cloudevents.v1;

// ...

message CloudEvent {
// ...
}

// ...

When using protoc this puts python code in io.cloudevents.v1.cloudevents_pb2 and assumes this location for imports from other python modules.

This then conflicts with the python builtin io module AFAICT: https://docs.python.org/3/library/io.html

It would be great to have some solution to this.

@aucampia
Copy link
Contributor

Copy link

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago.

@github-actions github-actions bot added the inactive Denotes the issue/PR has not seen activity in the last 90 days. label Mar 28, 2024
@brocaar
Copy link
Author

brocaar commented Mar 28, 2024

This issue is still relevant and needs a solution.

@github-actions github-actions bot removed the inactive Denotes the issue/PR has not seen activity in the last 90 days. label Mar 29, 2024
@chrisirhc
Copy link

Another way to approach this is to allow customizing the module prefix.
This makes generated code rules more useful since adding a module prefix is a common way to ensure that there's no module conflicts.

The module name is parsed by this helper:

std::string ModuleName(absl::string_view filename) {
std::string basename = StripProto(filename);
absl::StrReplaceAll({{"-", "_"}, {"/", "."}}, &basename);
return absl::StrCat(basename, "_pb2");
}

I suggested a similar fix on grpclib, see vmagamedov/grpclib#196 .

@chrisirhc
Copy link

chrisirhc commented Jun 30, 2024

Update: I made a PR to support a custom module prefix in the Python imports for protobuf but I'm not familiar with the edge cases of such a feature.
I also didn't see a lot of tests for the python plugin and import behavior so I can't easily tell if the approach works for usages. It could use a pair of eyes:

#17286

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests