Skip to content

Add support for generating python proto code #110

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Jan 21, 2025
Merged

Add support for generating python proto code #110

merged 7 commits into from
Jan 21, 2025

Conversation

coolkp
Copy link
Contributor

@coolkp coolkp commented Dec 18, 2024 •
edited
Loading

Changes in the following PR

  • Updated protoc version
  • Generate go code only has new protoc version
  • Update generate_go_protobuf -> generate_protobuf to add python code generation
  • Generated Python code
  • Package generated python code as pip package

@coolkp coolkp force-pushed the main branch 2 times, most recently from 3ca3253 to 5394619 Compare December 18, 2024 22:52
@coolkp
Add support for generating python proto code
e6266d0 
Signed-off-by: Kunjan Patel <kunjan@ucla.edu>
Signed-off-by: Kunjan Patel <kunjanp@google.com>
@coolkp coolkp marked this pull request as ready for review December 20, 2024 19:03
adisuissa
adisuissa reviewed Jan 3, 2025
View reviewed changes
Copy link

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

Thanks!
In addition to the comments, a high-level question: are the __init__.py files auto generated? (I'm assume the bazel command adds these, but just want to make sure).

coolkp added 4 commits January 8, 2025 01:59
@coolkp
Consolidate, setup.py with pyproject.toml and remove redundant licens…
a4db799 
…e. Add disrtribution wheel

Signed-off-by: Kunjan Patel <kunjanp@google.com>
@coolkp
Consolidate, setup.py with pyproject.toml and remove redundant licens…
e2c79e1 
…e. Add disrtribution wheel

Signed-off-by: Kunjan Patel <kunjanp@google.com>
@coolkp
Refactor generate_protobuf
01c7441 
Signed-off-by: Kunjan Patel <kunjanp@google.com>
@coolkp
Refactor generate_protobuf
382e8f0 
Signed-off-by: Kunjan Patel <kunjanp@google.com>
adisuissa
adisuissa reviewed Jan 15, 2025
View reviewed changes
Copy link

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

Thanks, overall LGTM.
Just a suggestion on how to refactor the function to support multi-languages.

@coolkp
Create class to hold language specific logic
8d1e7ac 
Signed-off-by: Kunjan Patel <kunjanp@google.com>
adisuissa
adisuissa reviewed Jan 15, 2025
View reviewed changes
Copy link

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

Thanks!
Overall LGTM, one comment about unifying requirements if possible.

tools/generate_lang_files_from_protos.py
input_dir = language_config.get_input_dir(bazel_bin, rule_dir)
input_files = glob.glob(os.path.join(input_dir, language_config.generated_file_pattern))
output_dir = os.path.join(output, rule_dir)
print(f"Moving {len(input_files)} generated files from {input_dir} to output_dir {output_dir}")
Copy link

Choose a reason for hiding this comment

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

nit:

Suggested change
print(f"Moving {len(input_files)} generated files from {input_dir} to output_dir {output_dir}")
print(f"Copying {len(input_files)} generated files from {input_dir} to output_dir {output_dir}")

Copy link

Choose a reason for hiding this comment

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

Not sure why it got resolved.

@coolkp
Delete the python package
f324a7f 
Signed-off-by: Kunjan Patel <kunjanp@google.com>
adisuissa
adisuissa approved these changes Jan 21, 2025
View reviewed changes
Copy link

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

I'm going to merge this to unblock you.
If there are other files that are not auto-generated, please remove them in a followup PR (and add them to the ignored list).

tools/generate_lang_files_from_protos.py
input_dir = language_config.get_input_dir(bazel_bin, rule_dir)
input_files = glob.glob(os.path.join(input_dir, language_config.generated_file_pattern))
output_dir = os.path.join(output, rule_dir)
print(f"Moving {len(input_files)} generated files from {input_dir} to output_dir {output_dir}")
Copy link

Choose a reason for hiding this comment

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

Not sure why it got resolved.

@adisuissa adisuissa merged commit 2f00578 into cncf:main Jan 21, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adisuissa adisuissa adisuissa approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@coolkp @adisuissa