-
Notifications
You must be signed in to change notification settings - Fork 8
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
HTTPX client #93
HTTPX client #93
Conversation
log10/load.py
Outdated
if target_service == "bigquery": | ||
return str(uuid.uuid4()) | ||
|
||
def post_request(url: str, json: dict = {}) -> requests.Response: |
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.
def post_request(url: str, json: dict = {}) -> requests.Response: | |
def post_request(url: str, json: dict = {}) -> httpx.Response: |
also remove the import requests
log10/load.py
Outdated
if target_service == "bigquery": | ||
return str(uuid.uuid4()) | ||
|
||
def post_request(url: str, json: dict = {}) -> requests.Response: |
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.
Since this swallows exceptions, should we rename to try_post_request
?
log10/load.py
Outdated
# | ||
# Store request | ||
# | ||
post_request(url, json=log_row) |
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.
Maybe post request should get moved out of the try/except since it already absorbs exceptions. That way the try/except can tightly wrap the LLM call.
output = None | ||
result_queue = queue.Queue() | ||
global last_completion_response | ||
global sessionID |
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.
Do these need to be global? We can address in a follow up PR.
log10/load.py
Outdated
""" | ||
Get session ID from log10. | ||
""" | ||
res = post_request(url + "/api/sessions", {}) |
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.
Since post_request swallows errors, we need to always check for None response eg if not res return None
log10/load.py
Outdated
# If we cannot get a completion id, continue with degraded functionality (no logging). | ||
# | ||
completion_url = url + "/api/completions" | ||
r = post_request(completion_url, json={}) |
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.
maybe it's worth to have a function to check response for all the post_request calls? We need to check if response is returned here to make sure that we can access the body.
No description provided.