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: adjust docs to new .post() behaviour #4344

Merged
merged 20 commits into from
Feb 16, 2022
Merged

docs: adjust docs to new .post() behaviour #4344

merged 20 commits into from
Feb 16, 2022

Conversation

JohannesMessner
Copy link
Contributor

@JohannesMessner JohannesMessner commented Feb 15, 2022

No description provided.

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

github-actions bot commented Feb 15, 2022

Latency summary

Current PR yields:

  • 😶 index QPS at 1156, delta to last 2 avg.: -2%
  • 🐎🐎🐎🐎 query QPS at 51, delta to last 2 avg.: +41%
  • 🐢🐢 avg flow time within 1.4672 seconds, delta to last 2 avg.: -10%
  • 🐢🐢 import jina within 0.3528 seconds, delta to last 2 avg.: -11%

Breakdown

Version Index QPS Query QPS Avg Flow Time (s) Import Time (s)
current 1156 51 1.4672 0.3528
2.7.0 1195 51 1.7288 0.3509
2.6.4 1185 20 1.5656 0.4457

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

@JohannesMessner JohannesMessner changed the title docs: adjust docs to now .post() behaviour docs: adjust docs to new .post() behaviour Feb 15, 2022
Copy link
Member

@JoanFM JoanFM left a comment

Choose a reason for hiding this comment

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

Make sure that request_size parameter is explained and considered also

docs/fundamentals/flow/flow-api.md Outdated Show resolved Hide resolved
@JoanFM JoanFM closed this Feb 15, 2022
@JoanFM JoanFM reopened this Feb 15, 2022
@JohannesMessner JohannesMessner marked this pull request as ready for review February 15, 2022 16:20
docs/fundamentals/flow/flow-api.md Outdated Show resolved Hide resolved
Copy link
Member

@JoanFM JoanFM left a comment

Choose a reason for hiding this comment

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

Should we also explain the return_responses or we should not? Also we are missing docs about the Response object right?

docs/fundamentals/flow/client.md Outdated Show resolved Hide resolved
docs/fundamentals/flow/client.md Outdated Show resolved Hide resolved
Co-authored-by: Joan Fontanals <joan.martinez@jina.ai>
@JohannesMessner
Copy link
Contributor Author

JohannesMessner commented Feb 15, 2022

Should we also explain the return_responses or we should not?

It is explained under 'returning results from .post()', does it need more explanation?

Also we are missing docs about the Response object right?

I need to change that, it should say DataRequest and not Response. But I don't think we wanted to only give a broad overview, without explaining every detail

@JoanFM
Copy link
Member

JoanFM commented Feb 15, 2022

Should we also explain the return_responses or we should not?

It is explained under 'returning results from .post()', does it need more explanation?

Also we are missing docs about the Response object right?

I need to change that, it should say DataRequest and not Response. But I don't think we wanted to only give a broad overview, without explaining every detail

It needs to be clear what can be found in the response object (not sure now what is the exact class). But the user needs to understand the attributes

@JohannesMessner JohannesMessner marked this pull request as draft February 16, 2022 09:03
@JohannesMessner JohannesMessner marked this pull request as ready for review February 16, 2022 09:43
@JohannesMessner JohannesMessner marked this pull request as draft February 16, 2022 09:47
@JohannesMessner JohannesMessner marked this pull request as ready for review February 16, 2022 09:58

There also exists an async version of the Python Client so that it can easily be used from an `asyncio` context.

While the standard `Client` is also asynchronous under the hood, its async version exposes this fact to the outside world,
Copy link
Member

Choose a reason for hiding this comment

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

what is meant by this?

Copy link
Member

Choose a reason for hiding this comment

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

I would mention, it allows to asyncrhonously iterate over responses as they come

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what is meant by this?

That the async version doesn't hide its async-ness, unlike the normal client. To be honest, I don't know all the details about this async client myself, i just kept this more or less unchanged from Tobi's version. Changed it slightly, let me know if it makes more sense to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise feel free to suggest and apply changes yourself of course

Copy link
Member

Choose a reason for hiding this comment

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

@jacobowitz what is meant with client also being async?

@github-actions
Copy link

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

@JoanFM JoanFM merged commit 85271f2 into master Feb 16, 2022
@JoanFM JoanFM deleted the docs-post-return branch February 16, 2022 11:10
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/L size/M size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants