vespa-engine / pyvespa

Python API for https://vespa.ai, the open big data serving engine
https://pyvespa.readthedocs.io/
Apache License 2.0
79 stars 24 forks source link

Switch Async HTTP-client to `httpx` (HTTP/2-support) #770

Closed thomasht86 closed 3 weeks ago

thomasht86 commented 1 month ago

I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.

⚠️ Encountered this bug when tenacity retry is applied

```python self = , wait=, before=, after=)> retry_state = def iter(self, retry_state: "RetryCallState") -> t.Union[DoAttempt, DoSleep, t.Any]: # noqa fut = retry_state.outcome if fut is None: if self.before is not None: self.before(retry_state) return DoAttempt() is_explicit_retry = fut.failed and isinstance(fut.exception(), TryAgain) if not (is_explicit_retry or self.retry(retry_state)): return fut.result() if self.after is not None: self.after(retry_state) self.statistics["delay_since_first_attempt"] = retry_state.seconds_since_start if self.stop(retry_state): if self.retry_error_callback: > return self.retry_error_callback(retry_state) E TypeError: 'staticmethod' object is not callable notebooks/lib/python3.9/site-packages/tenacity/__init__.py:322: TypeError ```

We should make unit tests for error handling. Maybe this is part of your fix?

jobergum commented 1 month ago

Last time I tried httpx with http/2 I ran into issues with that it did not handle the case when the server would shutdown the connection (we do in Vespa cloud to disallow extremely long connections)

thomasht86 commented 1 month ago

Last time I tried httpx with http/2 I ran into issues with that it did not handle the case when the server would shutdown the connection (we do in Vespa cloud to disallow extremely long connections)

That's very useful info. I will try to mock a test for that to check!

thomasht86 commented 3 weeks ago

Closing for now, as httpx did not improve feeding performance notably, even with HTTP/2. Need to investigate further to justify the change.