Skip to content

Conversation

@rwnx
Copy link

@rwnx rwnx commented Sep 25, 2020

This is a sensible thing, but specifically recommended by requests

https://2.python-requests.org/en/master/user/quickstart/#timeouts

Nearly all production code should use this parameter in nearly all requests.
Failure to do so can cause your program to hang indefinitely

this was introduced in requests 2.4.0 so i've added a version constraint
to setup.py to support this.

See https://requests.readthedocs.io/en/latest/community/updates/#id55

I'm using a constant in Algorithmia/client to do this. I want to make it clear that it's static config at the moment so as to keep a single source of truth.

This is a sensible thing, but specifically recommended by requests

https://2.python-requests.org/en/master/user/quickstart/#timeouts

> Nearly all production code should use this parameter in nearly all requests.
> Failure to do so can cause your program to hang indefinitely

this was introduced in requests 2.4.0 so i've added a version constraint
to setup.py to support this.

See https://requests.readthedocs.io/en/latest/community/updates/#id55
@rwnx rwnx marked this pull request as ready for review September 25, 2020 15:02
@zeryx zeryx self-requested a review December 9, 2020 17:46
Copy link
Contributor

@zeryx zeryx left a comment

Choose a reason for hiding this comment

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

Hey @jerometwell ! Thanks for the PR!
We typically provide a mechanism for the API requests themselves (especially algorithm requests) to have server side timeouts (https://algorithmia.com/developers/api/?python#invoke-an-algorithm); however I can understand the reasoning for why you may want to be able to set a timeout with the CLI.
I'd suggest in keeping with our current infrastructure design to maintain the server side timeout system, and to provide that timeout=... flag in the event that a user passes such a flag to the algo run .. operation.
With that kind of change I think that would make a great addition to our CLI and I'd be happy to merge it in :)

@rwnx
Copy link
Author

rwnx commented Dec 9, 2020

Hi @zeryx , Thanks for responding.
I hope i'm not misinterpreting what we're discussing here, I think there might be some crossed wires.

This won't interfere with the current timeout system at all - it has an entirely different function that relates to the socket behaviour and the raw http-y parts of this client.

I didn't really have any intention of it being configurable by the caller. My Intention was to set a sensible default for general network conditions so the library will not block indefinitely on a bad network connection/bad socket. As noted by the creator of the requests package, there's almost no reason to have this unset - Is there a reason we'd advocate for this behaviour?

@rwnx rwnx requested a review from zeryx December 10, 2020 11:14
@zeryx zeryx requested a review from kennydaniel December 10, 2020 16:08
@rwnx rwnx closed this by deleting the head repository Mar 17, 2024
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.

2 participants