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 custom HTTPClient class to be consumed by APIs #6

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

FlorianMF
Copy link
Contributor

I created a separate HTTP Client class which the APIs consume.
This way a (Management)API could take several clients or the same client be used by several APIs

@cceyda
Copy link
Owner

cceyda commented Jun 21, 2021

I'm a bit hesitant about this change;

  1. I would rather not have import streamlit statement (ie front end) in api.py. In case anyone wants to use just the APIs.
  2. I personally prefer avoiding wrapper classes that just do one simple thing. It is easier for the code reader/user/maintenance not to have to worry about a custom class. Also it is perfectly fine to have 2 different HTTP clients for different APIs (& maybe even better that way).

I think we can just allow passing an httpx.Client or kwargs instead of just an error callback for finer grained control (of timeouts etc) if you feel it is necessary. I used the error callback thing because that was the least verbose way.

InferenceAPI/dashboard should probably have their own separate logic and place (I will make some folders).
we can reuse & refactor as needed later on.

@FlorianMF
Copy link
Contributor Author

  1. That's a valid argument. In this case we could also simply move the HTTPClient to another file or let the user provide their own.
  2. The most elegant way would then be each API to have a default http client and allow the user to pass his own. I'm not sure if anyone would really care about the http client when using the dashboard. We should probably stick with a default client (with default callback) and allow the user to either pass a custom client or a custom callback.

Definitely, separating logically the APIs would be a good thing. Maybe this way?
--- dash
--- api
--- management_api.py
--- inference_api.py
--- utilities.py

@cceyda
Copy link
Owner

cceyda commented Jun 21, 2021

Lets shelve this until there is a more concrete InferenceAPI.
I opened this #11 for some ideas

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