-
Notifications
You must be signed in to change notification settings - Fork 13
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
Feature/api refactor #604
Feature/api refactor #604
Conversation
This reverts commit e9466db.
Suggestion for refactoring of overwall structure`
and include a stub The main interface for users should be the callse |
macrosynergy/download/dq_api.py
Outdated
while (not success) and (i < 3): | ||
js, success, self.last_url, msg = dq_request( | ||
url=self.__token_url, | ||
data=self.token_data, | ||
method="post", | ||
proxies=self.token_proxy, | ||
) | ||
i += 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we trying to request multiple times?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we are better off using off the shelves libraries for the OAuth handling:
from oauthlib.oauth2 import BackendApplicationClient
from requests_oauthlib import OAuth2Session
OAUTH_BASE_URL: str = "https://api-developer.jpmorgan.com/research/dataquery-authe/api/v2/"
OAUTH_TOKEN_URL: str = "https://authe.jpmchase.com/as/token.oauth2"
OAUTH_DQ_RESOURCE_ID: str = "JPMC:URI:RS-06785-DataQueryExternalApi-PROD"
class DQOAuth(object):
def __init__(self, client_id: str, client_secret: str):
self.client_id: str = client_id
self.client_secret = client_secret
self.client = BackendApplicationClient(client_id=client_id, aud=OAUTH_DQ_RESOURCE_ID)
self.session = OAuth2Session(client=client)
def _renew_token(self):
self.session.fetch_token(
token_url=OAUTH_TOKEN_URL,
client_id="<client_id>",
client_secret="<client_secret>"
)
def authorized(self):
return self.session.authorized
def request(url: str, method: = "GET"):
if not self.authorized():
self._renew_token()
with self.session.request(url=url, method=method) as r:
assert r.ok
js = r.json()
return js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic behind the multiple requests was to retry getting the token 3 times if and when it a token renew request fails. I've reverted to the original method (currently in use).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look into using the library methods directly. Seems cleaner to use as well.
…into feature/api_refactor
macrosynergy/download/exceptions.py
Outdated
class AuthenticationError(Exception): | ||
"""Raised when authentication fails.""" | ||
|
||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for the pass
here.
macrosynergy/download/dataquery.py
Outdated
logger.error( | ||
f"Invalid credentials." | ||
f"Request failed for URL: {last_url}" | ||
f" with message: {msg}" | ||
f" and response: {js}" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could pass parameters to the logger:
logger.error(
"Invalid credentials. Request failed for URL: %s with message: %s and response: %s",
last_url, msg, js
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logger.error(
"Invalid credentials. Request failed for URL: %s"
" with message: %s and response: %s",
last_url, msg, js
)
macrosynergy/download/dataquery.py
Outdated
(not (select in response.keys())) | ||
and (counter <= server_count) | ||
and (invalid_responses <= server_count) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't we arbitrarily break after 5 retries now as default? Even if the next link has more left?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes; that's true.
I think setting server_count=10
and testing (counter+invalid_responses)<=server_count
before breaking might be a better idea.
PS: the variables after refactor are
server_count
→max_retries
counter
→conxn_errors
elif count > 5: | ||
recursive_call = False | ||
logger.warning( | ||
f"Error requesting tickers: {', '.join(error_tickers)}. No longer retrying." | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be further up? in first error_tickers
?
|
||
log_url = f"{url}?{requests.compat.urlencode(params)}" if params else url | ||
log_url = requests.compat.quote(log_url, safe="%/:=&?~#+!$,;'@()*[]") | ||
logger.info(f"Requesting URL: {log_url} " + track_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will fail if track_id
is None:
msg = "Hello, World" + None
Throws a TypeError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah got it we convert track id to "" if None.
No description provided.