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

WIP: Feature/automated full interface generation #181

Conversation

markdoerr
Copy link
Contributor

This is the related pull request to issue #180.

It is still work in progress, because the issue #178 needs to fixed.

The workflow for the interface generation is the following:

cd <to my app directory containing the model.py>
then run
./manage.py generategrpcinterface <my_app>

This will generate a grpc directory, containing three files:
handler.py serializers.py and services.py

and fill it with default content.

Then the protogenerator needs to be executed:
./mange.py generateproto <my app> # this should work after fixing #178

Finally one needs to remove the comments in serializer.py to assign the
proto_class and proto_class_list

That's it :)

(I also added the official python api call of protoc - so no external shell call is required - but there is also something with the paths I need to fix)

Looking forward to your comments....

INDENT_WIDTH = getattr(settings, 'INDENT_WIDTH', 4)


class Command(LabelCommand):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's by app, couldn't it be AppCommand ? It would automatically handle some of your logic below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AppCommand

Looks like a good suggestion @legau . I will have a look into that...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see what this file has to do with the rest

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a hack that is required because of a bug in python protoc.
See:
protocolbuffers/protobuf#1491
and
grpc/grpc#29459

# )
# correcting protoc rel. import bug
#
(pb2_files, _) = os.path.splitext(os.path.basename(file_path))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer using pathlib for that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@legau - indeed, pathlib is the better choice. if you decide to use the buf-workflow for interface generation, this hack will be obsolete anyway. Let's discuss this tomorrow.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you provide the workflow you have in mind with this ? I have trouble understanding what are the requirements and what stage should you launch the command

Copy link
Contributor Author

@markdoerr markdoerr Jul 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, @legau,
the goal should be to generate as much of the gRPC API as possible automatically:

For this Merge Request, the workflow would look like:

add the django_socio_grpc app to the django settings of a new project with one or several apps

cd to my app directory containing the model.py
then run
./manage.py generategrpcinterface <my_app>

This will generate a grpc directory within the app, containing three files:
handler.py serializers.py and services.py

and fill it with default content.

Then the protogenerator needs to be executed:
./mange.py generateproto # this should work after fixing #178

Finally one needs to remove the comments in serializer.py to assign the
proto_class and proto_class_list

By that one would have a good starting point for further extensions of the interface.
This is extremly handy, if you have a project with many apps and many fields in the models.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future, one should consider the buf workflow for the .proto compilation / API generation.

Also custom services could be added based on the .proto file in the future.

file_path.parent.mkdir(parents=True, exist_ok=True)
self.check_or_write(file_path, proto, registry.app_name)

if self.generate_pb2:
if not settings.BASE_DIR:
raise ProtobufGenerationException(detail="No BASE_DIR in settings")
os.system(
f"python -m grpc_tools.protoc --proto_path={settings.BASE_DIR} --python_out=./ --grpc_python_out=./ {file_path}"
f"python -m grpc_tools.protoc --proto_path={proto_path} --python_out={proto_path} --grpc_python_out={proto_path} {file_path}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would this render your other PR obsolete ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point. Maybe we finish the first PR first :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dear Leni (@legau ),
I will rethink the workflow after what i learned from you today ...
(I think, the first PR still has it's validity, so let's complete this first.)

@AMontagu
Copy link
Contributor

@markdoerr after using a little bit more the library do you still think we need this ? Is yes can you describe your use case ?

@markdoerr
Copy link
Contributor Author

Dear @AMontagu and @legau,
after working for a longer time now with your framework, many things got much clearer - and I will try to make suggestions for improvements of the documentations that beginners have it easier.
Your DSG workflow is very handy and convenient.
For large applications with a lot of models (in LARA we have in the range of 100 - 150 models, I guess), an initial generator of the service.py, serializer.py and handler.py file comes quite handy.
We could consider combining the generators in the generateproto routine: if not these three files do not exist, just generate them and populate them. This would make the whole interface building even more convenient. What do you think ?

New Workflow:

  • build a new django app from scratch (e.g. the startproject or cookiecutter)
  • define the models
  • run "manage.py generateproto" -> this builds the whole DSG / gRPC infrastructure (handler.py, serializer.py, services.py, grpc folder with .proto and stubs)
  • update / refine models and / or add services and actions as required
  • run "mange.py generateproto" again
  • new proto files / subs are generated , but service.py / serializer.py / handler.py are not affected.

  • reiterate until one is happy with the models and the gRPC / DSG interface, then create an api folder in the project and let buf generate all the stubs also for grpc-web / go / or whatever ... (optionally in a CI/CD pipeline and a separate python api package)

What do you think ?

@legau
Copy link
Collaborator

legau commented Nov 29, 2023

Hi,
I guess that would be a nice addition but it should be its own command. Something like generategrpcservices maybe.

In your use case, what you would expect is to have it generate one service by model with one serializer and this service added to an app handler ?

@AMontagu
Copy link
Contributor

AMontagu commented Nov 30, 2023

I am not a fan of generating code from models. I mean this could have been done with DRF but it is not. You have https://pypi.org/project/drf-generators/ but it's an external dependencies.

we could totaly copy drf generators and call it dsg-generators and making the really small changes we need but what is generated is the most easy part of the app and when you start a project from the begining developer often code API endpoint after endpoint instead of defining all the models and then endpoints.

Specially considering that running the command after developer made change can bring obfuscated automatic code change that can lead to bug due to misunderstanding of the tool.

@markdoerr
Copy link
Contributor Author

Hi @AMontagu,
thanks for the hint - I was not aware of drf-generators.
I think it is a good idea, to copy the drf-generator code and adjust it to DSG needs.

Let's put it to the list of "nice-to-have" features of DSG :)

We could do this drf-generators "copy and adjustment", if there are more people interested in DSG to make the DSG start as painless as possible.

@markdoerr markdoerr closed this Dec 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants