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

Bugfix/reexport python type infos #8356

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fliiiix
Copy link
Contributor

@fliiiix fliiiix commented Jul 6, 2024

I tried to use the new .pyi files and stumbled over the re-export problem.
This change adds the classes in .pyi to __all__ to allow re-export.

Regenerated the grcp for python correctly i think that was missed with
3b27f53 and feels to me like an unwanted regression
in 1. generating the file in a different location and 2. generate it by default with a . in the file name which makes it nearly impossible to import with Python

@anton-bobukh 🔝 Did i misunderstand something?

And my clang-format didn't agree with some things.
@dbaileychess Is there a specific version which should be used?

clang-format --version
clang-format version 18.1.6

@github-actions github-actions bot added c++ codegen Involving generating code from schema grpc python labels Jul 6, 2024
@anton-bobukh
Copy link
Collaborator

Thanks for the fixes, @fliiiix!

Can you provide more details on the re-export issue? Why does the default behaviour – exporting everything that does not start with a _ – not work?

As for the dot in the filename, it looks like a bug. It should be an _ instead with the default being _fb, not .fb. Also, not sure about 1. generating the file in a different location, what do you mean by that?

@fliiiix
Copy link
Contributor Author

fliiiix commented Jul 8, 2024

re-export issue

Can you provide more details on the re-export issue?

I will look more into that, i'm not a 100% sure yet how it works.
I think it has something todo with the fact that the info is in a stub file.
(But maybe we are doing something weird in our setup 😅 )

Note that mypy treats stub files as if this is always disabled.
https://mypy.readthedocs.io/en/stable/config_file.html#confval-implicit_reexport

underscore issue

It should be an _ instead with the default being

Cool so i hopefully find time this week to provide a fix for that

grpc file location

generating the file in a different location

we have a portal file something like this:

namespace data;

/// Simple feedback for whether something worked or not
table SuccessReply {
  /// Set to true when Setting was successful
  success:bool;
}

/// Used when no data needs to be transferred
table Nil {
}

Which generates a folder data containing SuccessReply.py, Nil.py, ...

And before some change the data_grpc_fb.py was inside this data folder now i think it is just generated where ever flatc is run. (Right now i just 'fixed' that by moving the file back with cmake)

@fliiiix
Copy link
Contributor Author

fliiiix commented Jul 11, 2024

@anton-bobukh I marked this MR as Draft and opened #8359 for point 2 and 3.

I need some more time to analyze the re-export problem.
As far as I could tell the problem is the import in the .pyi files

from __future__ import annotations

import flatbuffers
import numpy as np

import flatbuffers
import typing
from portal_data.Nil import Nil # <- here this is i think considered re-exported under some conditions 

uoffset: typing.TypeAlias = flatbuffers.number_types.UOffsetTFlags.py_type

class Nil:
  ...
class NilT:
   ...

def Pack(self, builder: flatbuffers.Builder) -> None: ...
def NilStart(builder: flatbuffers.Builder) -> None: ...
def Start(builder: flatbuffers.Builder) -> None: ...
def NilEnd(builder: flatbuffers.Builder) -> uoffset: ...
def End(builder: flatbuffers.Builder) -> uoffset: ...

I'm not sure yet if the best approach is to remove the import or use this __all__ export or even do a from foo import bar as bar.

@fliiiix fliiiix force-pushed the bugfix/reexport-python-type-infos branch from 8b41104 to 7c06c55 Compare July 12, 2024 19:23
@github-actions github-actions bot removed the grpc label Jul 12, 2024
@fliiiix fliiiix force-pushed the bugfix/reexport-python-type-infos branch from 7c06c55 to 7d6aa5e Compare July 12, 2024 19:26
Importing the Base type for `--gen-object-api` in
`.pyi` files eg. `from namespace.Type import Type`
can lead to re-export issues depeding on how the
generated code is embeded.

To prevent this the import is removed since
the data Type is always defined just on top.
@fliiiix fliiiix force-pushed the bugfix/reexport-python-type-infos branch from 7d6aa5e to 2a1790e Compare July 13, 2024 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ codegen Involving generating code from schema python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants