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

Enhanced API calls: set User-Agent header & E-Utility rate limits #83

Merged
merged 7 commits into from
Dec 7, 2018

Conversation

dhimmel
Copy link
Member

@dhimmel dhimmel commented Dec 5, 2018

Adds new manubot contact email to user-agent, which can be helpful so resources can contact us regarding our API requests. Adds platform and python version info to user-agent so the upcoming translation-server can monitor usage.

On my system, this produced:
manubot/0.2.1 (Linux; Python/3.6) <contact@manubot.org>
@dhimmel
Copy link
Member Author

dhimmel commented Dec 5, 2018

Interesting that the AppVeyor build failed the test: test_get_pmid_for_doi[10.1161/CIRCGENETICS.115.001181-27094199]. This was due to the logged message:

pubmed.py                  282 WARNING  Status code 429 querying https://eutils.ncbi.nlm.nih.gov/entrez/eutils/esearch.fcgi?db=pubmed&term=10.1161%2FCIRCGENETICS.115.001181%5BDOI%5D

Status code 429 refers to too many requests.

@dhimmel dhimmel requested a review from dongbohu December 5, 2018 17:25
@dhimmel
Copy link
Member Author

dhimmel commented Dec 5, 2018

@agitter I suggested @dongbohu as a reviewer who is a backend dev for the Greene Lab.

@agitter
Copy link
Member

agitter commented Dec 5, 2018

@agitter I suggested @dongbohu as a reviewer who is a backend dev for the Greene Lab.

Thanks. When there are potential alternate reviewers I appreciate having them help out. I'll still watch and casually review all pull requests.

Status code 429 refers to too many requests.

Is this an issue? Do we know how many requests are allowed?

Adopt sytem specific files from manubot/rootstock@135f451
With the addition of .vscode
@dhimmel
Copy link
Member Author

dhimmel commented Dec 6, 2018

Looks like the issue could be that E-utilities API has a limit of 3 requests per second. More info here. If you register for an API key, that increases to 10 requests per second. I wonder why this was never an issue in the past, but perhaps some aspect of the tests on AppVeyor got faster, which is causing us to hit the limit. I'll look into a workaround.

@agitter
Copy link
Member

agitter commented Dec 6, 2018

This is a naive idea, but could it be related to AppVeyor running tests in parallel in multiple environments?

@dhimmel
Copy link
Member Author

dhimmel commented Dec 6, 2018

@agitter I think 27fae1d fixed it, so that's great. Doesn't neccessarily rule out tests being run in parallel, but if so I am not sure whether 27fae1d is a complete solution.

@dhimmel dhimmel changed the title Make get_manubot_user_agent a generic util function Enhanced API calls: set User-Agent header & E-Utility rate limits Dec 6, 2018
Copy link

@dongbohu dongbohu left a comment

Choose a reason for hiding this comment

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

I got one trivial comment.

@@ -241,7 +245,7 @@ def get_pmcid_and_pmid_for_doi(doi):
return {}
try:
element_tree = xml.etree.ElementTree.fromstring(response.text)
except Exception as error:
except Exception:
Copy link

Choose a reason for hiding this comment

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

Don't you want to include the exception as a string in the logger?

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case, I don't think the exception will be helpful, because it will be a very verbose and unwieldy stacktrace that the XML could not be parsed. We log response.text, which will usually contain a non-XML error message that is actually relevant.

Copy link

Choose a reason for hiding this comment

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

Sounds good. I am going to approve this PR.

@dhimmel
Copy link
Member Author

dhimmel commented Dec 7, 2018

@agitter does this PR look okay? I don't think you've seen it since I added the rate limiter for E-Utility queries.

Copy link
Member

@agitter agitter left a comment

Choose a reason for hiding this comment

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

The ratelimiter package is a great solution.

@dhimmel
Copy link
Member Author

dhimmel commented Dec 7, 2018

The ratelimiter package is a great solution.

CC @RazerM... thanks! The context manager is helpful since we can apply a single ratelimiter across calls in multiple functions.

@dhimmel dhimmel merged commit 7440a8b into manubot:master Dec 7, 2018
@dhimmel dhimmel deleted the email branch December 7, 2018 15:15
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.

3 participants