close
Skip to content

Rework grpc locations and add some different structure#2

Closed
cretz wants to merge 6 commits into
temporalio:mainfrom
cretz:structure1
Closed

Rework grpc locations and add some different structure#2
cretz wants to merge 6 commits into
temporalio:mainfrom
cretz:structure1

Conversation

@cretz
Copy link
Copy Markdown
Contributor

@cretz cretz commented Jan 27, 2022

What was changed

  • temporalio/proto/temporal/api -> temporalio/api
  • temporalio/proto/dependencies -> temporalio/api/dependencies
  • temporalio/proto/temporal/sdk -> temporalio/bridge/proto
  • Add aliases for all protos/grpc to top-level of respective module for clarity
  • Add LICENSE
  • Add simple pure-python gRPC test
  • Add isort to format/lint task
  • Add mypy lint check and mypy-protobuf support
  • Add pytest task

@cretz cretz requested review from Sushisource and bergundy January 27, 2022 21:37
@cretz cretz marked this pull request as draft January 27, 2022 21:38
Comment thread pyproject.toml Outdated
Comment thread pyproject.toml Outdated
Comment thread scripts/gen-protos.py
Comment thread scripts/gen-protos.py
@@ -0,0 +1,54 @@
import logging
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can test just the protos since as I mentioned before we don't need to generate the grpc output.

Copy link
Copy Markdown
Contributor Author

@cretz cretz Jan 27, 2022

Choose a reason for hiding this comment

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

I think it's reasonable not only to confirm the pure Python client works, but to show even what using a pure python gRPC client looks like. Not committing protos makes discovery horrible for code/repo readers.

@cretz cretz marked this pull request as ready for review January 27, 2022 23:15
@@ -0,0 +1 @@
from .core_interface_pb2 import ActivityHeartbeat, ActivityTaskCompletion
Copy link
Copy Markdown
Contributor Author

@cretz cretz Jan 27, 2022

Choose a reason for hiding this comment

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

This is part of the auto-generated code, but I think it still deserves to be checked in to keep the dir here. But if there's disagreement, I can remove this and add .gitkeep to this dir instead (or just mkdir it during build).

Comment thread tests/api/test_grpc_stub.py
Comment thread README.md
- Build the project

```bash
poe build
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There's also a poetry build command, I think we could integrate with that.
Not blocking this PR.

Comment thread pyproject.toml
[tool.poetry.dependencies]
python = "^3.7"
grpcio = "^1.43.0"
python = "^3.7"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we want to restrict to python<3.10?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Why not 3.10? I use 3.10 successfully on Windows.

Comment thread scripts/gen-protos.py
Comment on lines +65 to +68
with (base_path / "__init__.py").open("w") as f:
for stem, messages in imports.items():
for message in messages:
f.write(f"from .{stem} import {message}\n")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a nice addition!

Comment thread scripts/gen-protos.py
Comment on lines +53 to +55
content = fix_api_import(content)
content = fix_dependency_import(content)
content = fix_sdk_import(content)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: you can use a single re.sub call with a callback https://docs.python.org/3/library/re.html#re.sub
Not blocking the PR of course.

Comment thread scripts/gen-protos.py
Comment thread scripts/gen-protos.py
Comment thread tests/api/test_grpc_stub.py Outdated
Comment thread tests/api/test_grpc_stub.py Outdated
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.

2 participants