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

Add timeout to requests.get in _citation.py #1091

Merged
merged 3 commits into from
May 6, 2021
Merged

Add timeout to requests.get in _citation.py #1091

merged 3 commits into from
May 6, 2021

Conversation

SarahAlidoost
Copy link
Contributor

@SarahAlidoost SarahAlidoost commented Apr 28, 2021

Description

Closes #1088

Link to documentation:


Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the πŸ›  Technical or πŸ§ͺ Scientific review.


To help with the number pull requests:

@zklaus
Copy link
Contributor

zklaus commented Apr 29, 2021

The requests documentation here and here certainly seems to suggest using timeouts.

I feel like 1 second is a bit short, but looking around the intertubes that seems to be a feeling that is not widely shared.

Perhaps it would be nice to establish a universal network timeout as a configuration parameter?

@SarahAlidoost
Copy link
Contributor Author

The requests documentation here and here certainly seems to suggest using timeouts.

I feel like 1 second is a bit short, but looking around the intertubes that seems to be a feeling that is not widely shared.

Thank you. The timeout=1s was tested on my machine. However, it is now increased to 5s.

Perhaps it would be nice to establish a universal network timeout as a configuration parameter?

Do you mean as a parameter to config-user-example.yml?

@SarahAlidoost SarahAlidoost marked this pull request as ready for review May 6, 2021 08:27
@SarahAlidoost SarahAlidoost requested a review from zklaus May 6, 2021 08:48
Copy link
Contributor

@zklaus zklaus left a comment

Choose a reason for hiding this comment

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

LGTM

@zklaus
Copy link
Contributor

zklaus commented May 6, 2021

Perhaps it would be nice to establish a universal network timeout as a configuration parameter?

Do you mean as a parameter to config-user-example.yml?

Yes, something like that. However, grepping through the code, I didn't find another place where we directly make requests, so for now I think it might not be necessary.

@SarahAlidoost
Copy link
Contributor Author

LGTM

I cannot merge the PR, could you please do it?

@schlunma schlunma merged commit 7b0f882 into master May 6, 2021
@schlunma schlunma deleted the fix_citation branch May 6, 2021 09:52
@zklaus zklaus added the enhancement New feature or request label Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slow _get_response in citation.py
3 participants