Custom Web UI support for API Server #839
Conversation
bentoml/saved_bundle/loader.py
Outdated
| @@ -177,6 +177,7 @@ def load(bundle_path): | |||
|
|
|||
| svc_cls = load_bento_service_class(bundle_path) | |||
| svc = svc_cls() | |||
| svc._path = bundle_path | |||
There was a problem hiding this comment.
I am not sure about this, there may be a better way of getting the bundle path.
There was a problem hiding this comment.
you can use svc._bento_service_bundle_path, which is equivalent and used for loading artifacts
bentoml/server/bento_api_server.py
Outdated
| mimetype="text/html", | ||
| ) | ||
| if hasattr(self.bento_service, '_static_files'): | ||
| return render_template('index.html') |
There was a problem hiding this comment.
In case custom UI is enabled, do we want to also have swagger UI on some different endpoint?
There was a problem hiding this comment.
Yea, that'd be great for debugging, I think we can add a /swagger.html endpoint which always renders the swagger UI
There was a problem hiding this comment.
Does it actually need the redner_template method? the user can not really use the templating functionality right? I'm more inclined to simply put all the static files under the static directory if they are just static files
There was a problem hiding this comment.
Makes sense!
so now the dir structure will be:
webui_dir/
|-- js/
|--css/
|--imgs/
|--index.html
Here webui_dir is the supplied path by the user.
instead of render_template now it uses send_from_directory
There was a problem hiding this comment.
Yea, that'd be great for debugging, I think we can add a
/swagger.htmlendpoint which always renders the swagger UI
In my opinion, /docs endpoint would be better for swagger UI since this is the same as what fastapi uses. And also if in future we change from swagger to something else, the path can remain the same.
So /docs for the HTML page and /docs.json for openapi json.
What do you think?
There was a problem hiding this comment.
I believe it should serve all files under the static_folder directory not just index.html, you may also need to specify static_url_path:
app = Flask(__name__, static_folder='../web_client', static_url_path='/')
and in your index view function do something likeapp.send_static_file('index.html')
yeh, this is working currently!
Adding static_url_path might break the swagger js/css files tho, you may need to adjust accordingly
Fixed that using send_from_directory and changing swagger static dir name.
I think this way it is more friendly to the web frontend developer, for example, if the user creates the frontend project with something like https://github.com/facebook/create-react-app or use webpack to build their js project, they can simply supply the built
distdirectory for BentoML to serve as static files, and everything should just work.
Yep, I am trying to achieve this...currently the only issue is serving any_path/index.html for any_path/, any_path/index.html and also any_path/index.
trying to find some better solution instead of just writing down all the cases.
There was a problem hiding this comment.
One another doubt is - should we allow user-defined routes to override the system defined (predefined by bento)?
currently, if the user-defined function name is index then the server doesn't start, so should we allow to override that?
this may provide some extra flexibility but may even be a bit confusing at times.
There was a problem hiding this comment.
currently, if the user-defined function name is index then the server doesn't start, so should we allow to override that?
.. the server doesn't start .. is that because it has added two rules handling the same path? I think it is probably not a good idea to allow user-defined @api to override the system ones. Maybe we should validate the @api name and make sure it is not in the reserved name list, such as index, healthz, metrics etc. Currently there's only a isidentifier check, you can add this validation here: https://github.com/bentoml/BentoML/blob/v0.8.2/bentoml/service.py#L311
There was a problem hiding this comment.
.. the server doesn't start ..is that because it has added two rules handling the same path?
Yep
Okey great!
should I include this in the same PR or create another issue/PR for this?
There was a problem hiding this comment.
@Korusuke a smaller PR that adds the validation would be nice, but it's ok to just include it in this PR as well, whichever is easier for you
Codecov Report
@@ Coverage Diff @@
## master #839 +/- ##
==========================================
+ Coverage 55.89% 60.65% +4.76%
==========================================
Files 114 118 +4
Lines 8400 8002 -398
==========================================
+ Hits 4695 4854 +159
+ Misses 3705 3148 -557
Continue to review full report at Codecov.
|
Korusuke
changed the title
bentoml/service.py
Outdated
| @@ -553,6 +574,10 @@ def artifacts(self): | |||
| def env(self): | |||
| return self._env | |||
|
|
|||
| @property | |||
| def webui(self): | |||
There was a problem hiding this comment.
webui and _static_files here seems to be referencing the path to web UI static files. maybe rename both to just web_static_content? This API technically can also be used to host other static files right? e.g. it can be a just a markdown documentation page for using this API and allow end-user to download example input json/image files
bentoml/server/bento_api_server.py
Outdated
| @@ -107,15 +117,18 @@ def start(self): | |||
| self.app.run(port=self.port, threaded=False) | |||
|
|
|||
| @staticmethod | |||
| def index_view_func(): | |||
| def index_view_func(bento_service): | |||
There was a problem hiding this comment.
consider refactor this to two different functions? swagger_ui_func and user_defined_ui_func, and decide which function to use in the setup_routes method?
|
There are a bunch of new changes to try and support everything. Below are some reasons for doing what I have done!
Supported paths for any type of static files:
Known unsupported paths:
Edit: Just to be clear |
|
I think the tests failed since So should I add a check? or define |
bentoml/server/bento_api_server.py
Outdated
|
|
||
| self.app = Flask( | ||
| self.static_path = os.path.join( | ||
| self.bento_service._bento_service_bundle_path, |
There was a problem hiding this comment.
_bento_service_bundle_path is the only set when loading from a saved BentoService bundle.
Maybe you could add a method get_web_static_content_path:
def get_web_static_content_path(self):
if self.bento_service._bento_service_bundle_path:
return os.path.join(
self.bento_service._bento_service_bundle_path, 'web_static_content'
)
elif: bento_service.web_static_content:
return os.path.join(os.getcwd(), bento_service.web_static_content)
else:
return None / raise ?
There was a problem hiding this comment.
This should fix the error you see in the unit test, it will also allow the user to test the BentoAPIServer without saving and loading, this can be super useful for debugging in an interactive session or Jupyter notebook
There was a problem hiding this comment.
This is perfect!!
I wanted something like this but was not sure how to implement it😅
| @@ -172,8 +200,29 @@ def setup_routes(self): | |||
| /classify | |||
| /predict | |||
| """ | |||
| if self.static_path: | |||
| self.app.add_url_rule( | |||
There was a problem hiding this comment.
Could you add some inline comments about these three url_rule here? reading this code it is not immediately clear to me what it does
There was a problem hiding this comment.
Ah, sorry...this is a neccessary evil😅 as flask doesn't handle it automagically.
# serve static files for any given path
# this will also serve index.html from directory /any_path/
# for path as /any_path/
self.app.add_url_rule(
"/<path:file_path>",
"static_proxy",
partial(self.static_serve, self.static_path),
)
# serve index.html from the directory /any_path
# for path as /any_path/index
self.app.add_url_rule(
"/<path:file_path>/index",
"static_proxy2",
partial(self.static_serve, self.static_path),
)
# serve index.html from root directory for path as /
self.app.add_url_rule(
"/", "index", partial(self.index_view_func, self.static_path)
)Any changes to this? Sorry but I am still trying to improve my commenting and documentation skills.
There was a problem hiding this comment.
@Korusuke that looks great, adding a little context here helps a lot!
bentoml/service.py
Outdated
|
|
||
| @property | ||
| def get_web_static_content_path(self): | ||
| if self._bento_service_bundle_path and self.name: |
There was a problem hiding this comment.
This should be if self.web_static_content and self._bento_service_bundle_path to make BentoService that does not have web_static_content to show an index page properly right?
self.name should always be presented
There was a problem hiding this comment.
This should be
if self.web_static_content and self._bento_service_bundle_pathto make BentoService that does not haveweb_static_contentto show an index page properly right?
Yeh, sorry missed that in the final commit
self.name should always be presented
I swear previously I was getting an error on self.name but now it's working😅
I think I may have used _bento_service_name while testing and cause that was giving error I replaced everything with self.name and then never checked it properly.
bentoml/service.py
Outdated
| Args: | ||
| web_static_content: path to directory containg index.html and static dir | ||
|
|
||
| >>> @webui('./ui/') |
There was a problem hiding this comment.
I'm a bit inclined to just name it @web_static_content('./ui/') as the user-facing API, thoughts?
There was a problem hiding this comment.
I am fine either way, will change it to web_static_content
There was a problem hiding this comment.
sounds good! I was thinking that users can potentially use this to host other files other than web UI files, e.g., using this to host test input files, JSON config file(some use cases require exposing server-side config to the client), or even pdf documentation, etc.
There was a problem hiding this comment.
Makes sense, the only downside is users currently cannot specify custom / path. So by default bento will look for index.html.
So for example, if someone is trying to serve index.json at /, then this won't work but /index.json will work.
We can add option to customize this, but I am more inclined not to as for production deployment it is better to just use nginx or something as serving static files via flask is both slow and processor intensive.
bentoml/saved_bundle/bundler.py
Outdated
| dest_web_static_content_dir = os.path.join( | ||
| module_base_path, 'web_static_content' | ||
| ) | ||
| # os.mkdir(dest_web_dir) |
| # copy custom web_static_content if enabled | ||
| if bento_service.web_static_content: | ||
| src_web_static_content_dir = os.path.join( | ||
| os.getcwd(), bento_service.web_static_content |
There was a problem hiding this comment.
add a os.path.isdir(src_web_static_content_dir) check here? , would be great to also support absolute path
There was a problem hiding this comment.
Added, it already supports absolute path. os.path.join is very smart!
| return self._web_static_content | ||
|
|
||
| @property | ||
| def get_web_static_content_path(self): |
There was a problem hiding this comment.
usually property won't have the verb get_, I feel making this a method instead of a property would be better
|
@Korusuke finished a pass with a few minor comments, otherwise this looks great! It would be great if you could add your web UI demo to the bentoml/gallery repository, it looks super cool! |
|
@Korusuke Thanks again for the awesome contribution, can't wait to try out building some ML-powered web apps with this feature. Merging now! |
|
Would be great to add some integration test for this later, currently, the related test infra are still working-in-progress #855 |
* Custom web UI * Fix _path * Add _static_files property * static serve * Refactor * Fix tests * Address comments
Description
Closes #739
Requires a directory to be passed containing
index.htmlandstaticdir.Note: Any files not inside
staticwill not be accessible.Example deployment: https://bentoml-her0ku-mtu5mtkwnjq0nao.herokuapp.com/
The example deployment is still WIP and has no loading indicator/error handling.
Please recommend changes for the demo deployment so the same can be used for documentation.
Motivation and Context
Implements #739
How Has This Been Tested?
Tested locally and deployed on Heroku
Types of changes
Component(s) if applicable
Checklist:
./dev/format.shand./dev/lint.shscript have passed(instructions).