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

Converting to async - Initial flow #2

Merged
merged 19 commits into from
May 28, 2024
Merged

Conversation

avik-taranis
Copy link

No description provided.

@avik-taranis avik-taranis requested a review from razsinay May 27, 2024 14:25
Copy link

@razsinay razsinay left a comment

Choose a reason for hiding this comment

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

I think it is worth adding so documentation to the functions, maybe describe inside the function the general flow, as this code is not trivial.


ARG BUILD_LOCAL

COPY requirements.txt test_requirements.txt setup.py README.md ./

Choose a reason for hiding this comment

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

Why do we need to copy the README.md?

Copy link
Author

Choose a reason for hiding this comment

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

Due to the long description in setup.py:

with open("README.md", "r") as fh: long_description = fh.read()


optional_any = self._wrap_rpc_behavior(continuation(handler_call_details), metrics_wrapper)
async for obj in behavior(request_or_iterator, servicer_context):

Choose a reason for hiding this comment

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

I suggest to find a better name for obj

yield obj
raise e

if response_streaming:

Choose a reason for hiding this comment

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

I am a bit confused with the multiple try/except blocks.
Maybe we can wrap some of the parts in functions?

Copy link
Author

Choose a reason for hiding this comment

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

Agree - opening an issue for it.

@avik-taranis avik-taranis merged commit 9c8312a into master May 28, 2024
12 checks passed
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.

3 participants