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

docs: add explain on return #4351

Merged
merged 2 commits into from
Feb 16, 2022
Merged

docs: add explain on return #4351

merged 2 commits into from
Feb 16, 2022

Conversation

hanxiao
Copy link
Member

@hanxiao hanxiao commented Feb 16, 2022

Pull Request Title

Description

Please describe what your PR is doing and why. Are there any parts which need extra attention during review? Are there any dependencies from other PRs or projcts? Is this a breaking change?

Is this PR ready or work in progress (WIP)? Ready means it can be reviewed and merged from the author's perspective. If the PR is WIP: Make it a draft PR and state open questions and TODO items.

Closes # (issue)

@@ -147,7 +147,7 @@ class MyExecutor(Executor):
@requests
async def foo(
self, docs: DocumentArray, parameters: Dict, docs_matrix: List[DocumentArray]
) -> Optional[Union[DocumentArray, Dict]]:
) -> Union[DocumentArray, Dict, None]:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
) -> Union[DocumentArray, Dict, None]:
) -> Optional[Union[DocumentArray, Dict]]:

This is by definition Optional and is much more standard

Copy link
Member Author

@hanxiao hanxiao Feb 16, 2022

Choose a reason for hiding this comment

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

actually it is the oppsite, if you look typing, pydantic or dataclass, they all get translate Optional into Union[..., None]

Copy link
Member Author

@hanxiao hanxiao Feb 16, 2022

Choose a reason for hiding this comment

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

Here this usage can better map to what I later said, "there are 3 ways of returning", Union[1,2,3] -> 3 ways

@github-actions github-actions bot added size/S area/docs This issue/PR affects the docs labels Feb 16, 2022
@github-actions
Copy link

github-actions bot commented Feb 16, 2022

Latency summary

Current PR yields:

  • 🐢🐢 index QPS at 817, delta to last 2 avg.: -22%
  • 🐎🐎 query QPS at 34, delta to last 2 avg.: +7%
  • 🐎🐎🐎🐎 avg flow time within 1.8541 seconds, delta to last 2 avg.: +15%
  • 🐎🐎 import jina within 0.4771 seconds, delta to last 2 avg.: +10%

Breakdown

Version Index QPS Query QPS Avg Flow Time (s) Import Time (s)
current 817 34 1.8541 0.4771
2.7.0 880 37 1.8483 0.4475
2.6.4 1240 25 1.3687 0.4177

Backed by latency-tracking. Further commits will update this comment.

Co-authored-by: Johannes Messner <44071807+JohannesMessner@users.noreply.github.com>
@github-actions
Copy link

📝 Docs are deployed on https://fix-docs-return--jina-docs.netlify.app 🎉

@JoanFM JoanFM merged commit 56eb755 into master Feb 16, 2022
@JoanFM JoanFM deleted the fix-docs-return branch February 16, 2022 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docs This issue/PR affects the docs size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants