torchserve-dashboard icon indicating copy to clipboard operation
torchserve-dashboard copied to clipboard

Add custom HTTPClient class to be consumed by APIs

Open FlorianMF opened this issue 3 years ago • 3 comments

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

FlorianMF avatar Jun 20 '21 15:06 FlorianMF

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.

cceyda avatar Jun 21 '21 06:06 cceyda

  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

FlorianMF avatar Jun 21 '21 07:06 FlorianMF

Lets shelve this until there is a more concrete InferenceAPI. I opened this https://github.com/cceyda/torchserve-dashboard/pull/11 for some ideas

cceyda avatar Jun 21 '21 13:06 cceyda