Validator improvements: use a httpx session/client and a custom User-Agent
#1188
Conversation
ml-evs
added
enhancement
|
This PR should probably only be merged once the client is ready, before a 0.18 release. |
| @@ -13,6 +13,7 @@ | |||
| """ | |||
|
|
|||
| import time | |||
|
|
|||
There was a problem hiding this comment.
Is this necessary? I think it might be nice to use isort as a pre-commit hook? Then all import statements will be unified for all :)
There was a problem hiding this comment.
This is obviously just a typo, but yeah, we should probably start using isort. Will remove this diff
| """Close the underlying httpx client, if it is still open.""" | ||
| try: | ||
| self.http_client.close() | ||
| except Exception: |
There was a problem hiding this comment.
Isn't there an explicit exception for this to use instead, so that unexpected exceptions are not missed?
There was a problem hiding this comment.
I was in two minds here really, the exceptions raised by close() are not documented so I don't think we can necessarily rely on them. The method already has handling for closing already closed connections (it just skips) so I think this is safe enough.
There was a problem hiding this comment.
There was a problem hiding this comment.
To me this seems more like you should simply call self.http_client.close() and that's that? No try/except. If an exception is raised it's something to be concerned about, if not, then it's already closed or it was successfully closed.
| sys.exit( | ||
| f"Unable to make request on {self.last_request}, did you mean http://{self.last_request}?" | ||
| f"Unable to make request on {self.last_request!r}, did you mean 'https://{self.last_request}'?\nFull error: {exc}" |
There was a problem hiding this comment.
When running the validator, should we have an outer try/except (perhaps for a custom catch-'em-all ValidatorException or similar?) to always add this extra Full error part instead? Then we can just raise with an appropriate error message here instead.
There was a problem hiding this comment.
We already have this with InternalError being "anything unexpected", but this is the one case where we want to end all further requests as the URL will remain malformed for the duration. This is unrelated to this PR really
There was a problem hiding this comment.
This is unrelated to this PR really
Sure - the majority of these comments are...
We already have this with
InternalErrorbeing "anything unexpected", but this is the one case where we want to end all further requests as the URL will remain malformed for the duration.
Okay - I thought it would always simply stop if something is raised, but I guess the catch-'em-all try/except simply collects exceptions then, instead of stopping? Which is definitely the way it should work in general.
Maybe then instead we could provide the exception with an agreed extra property, like hard_fail or force_fail or end, which the "outer" try/except checks before then either stopping or collecting and moving on?
And maybe this is a moot point. I'm just generally disliking using sys.exit() in a place that doesn't also directly handle terminal stuff. In a class like this it's weird - especially if testing this code through unit testing, etc.
| finally: | ||
| try: | ||
| self.client.close() | ||
| except Exception: |
There was a problem hiding this comment.
Again, maybe a specific exception here? I understand you at all costs want to close it, but passing on an all exceptions won't necessarily do that.
There was a problem hiding this comment.
See above.
Also in this case self.client can be a custom class passed by the user (we use the starlette test client in our tests) so I think we are forced to keep it general.
|
I've been trying to unpick the requests dependency in testing, but its proving quite tricky... the underlying starlette TestClient uses requests, and a lot of our testing mechanisms rely on mocking I'm going to pick out the user-agent commit and make a new PR, will leave the rest of this on hold for now, perhaps until starlette support httpx (same authors) - see encode/starlette#652 |
Either that, or you can check out the setup I have for OPTIMADE Gateway (https://github.com/Materials-Consortia/optimade-gateway/blob/main/tests/conftest.py#L90, as well as the rest of the test files). |
One for another day ;) |
|
I will have to refactor some of the test client in #1154 anyway, so maybe I'll find the motivation to integrate this then |
This PR drops the
requestsdependency and replaces it with httpx, which will allow async applications in the future (and is used by the upcoming client).It also:
httpx.Client(equivalent to arequests.Session) which seems to speed-up validation by 10-20% for odbx (will observe build time differences for the dashboard).User-Agentfor the validator that can be used by databases to prioritise/drop requests (closes The validator should use a customUser-Agentheader #1187).