refactor(sdk&framework): [Broken] framework runners #2288

bojiang · 2022-02-18T04:09:19Z

Description

Motivation and Context

How Has This Been Tested?

Checklist:

My code follows the bentoml code style, both make format and
make lint script have passed
(instructions).
My change reduces project test coverage and requires unit tests to be added
I have added unit tests covering my code change
My change requires a change to the documentation
I have updated the documentation accordingly

codecov · 2022-02-18T06:24:03Z

Codecov Report

Merging #2288 (2dc2318) into main (4308c67) will decrease coverage by 2.66%.
The diff coverage is 57.23%.

❗ Current head 2dc2318 differs from pull request most recent head 0f8420b. Consider uploading reports for the commit 0f8420b to get more accurate results

@@            Coverage Diff             @@
##             main    #2288      +/-   ##
==========================================
- Coverage   47.64%   44.98%   -2.67%     
==========================================
  Files         105      106       +1     
  Lines        8196     8448     +252     
==========================================
- Hits         3905     3800     -105     
- Misses       4291     4648     +357

Impacted Files	Coverage Δ
bentoml/_internal/context.py	`0.00% <0.00%> (ø)`
bentoml/_internal/external_typing/__init__.py	`0.00% <0.00%> (ø)`
bentoml/_internal/frameworks/tensorflow_v1.py	`0.00% <0.00%> (ø)`
bentoml/_internal/runner/local.py	`0.00% <0.00%> (ø)`
bentoml/_internal/runner/remote.py	`0.00% <0.00%> (ø)`
bentoml/_internal/server/service_app.py	`0.00% <0.00%> (ø)`
bentoml/_internal/frameworks/onnxmlir.py	`72.28% <33.33%> (+9.13%)`	⬆️
bentoml/_internal/frameworks/statsmodels.py	`72.16% <38.88%> (-26.43%)`	⬇️
bentoml/_internal/frameworks/keras.py	`70.92% <40.00%> (-27.12%)`	⬇️
bentoml/_internal/frameworks/paddle.py	`60.99% <40.00%> (-29.75%)`	⬇️
... and 32 more

aarnphm · 2022-02-18T19:06:16Z

we are removing load_runner ?

parano · 2022-02-18T22:06:40Z

bentoml/_internal/frameworks/detectron.py

@@ -3,6 +3,7 @@
 from typing import TYPE_CHECKING

 import numpy as np
+from torch._C import device


should this be in the conditional import try block?

parano · 2022-02-18T22:08:56Z

bentoml/_internal/frameworks/easyocr.py

 MODULE_NAME = "bentoml.easyocr"


 @inject
 def load(
-    tag: str,
+    tag: t.Union[str, Tag],


should we make a type alias TagLike?

parano · 2022-02-18T22:23:28Z

bentoml/_internal/frameworks/keras.py

@@ -403,11 +377,8 @@ def load_runner(
    *,
    predict_fn_name: str = "predict",
    device_id: str = "CPU:0",
-    predict_kwargs: t.Optional[t.Dict[str, t.Any]] = None,
+    partial_kwargs: t.Optional[t.Dict[str, t.Any]] = None,


also update in the doctoring below

parano · 2022-02-21T04:37:54Z

docker/utils.py

@@ -8,6 +8,11 @@
 from types import FunctionType
 from functools import reduce

+if sys.version_info[0] == 3 and sys.version_info[1] >= 8:


parano · 2022-02-21T07:10:39Z

bentoml/_internal/frameworks/catboost.py

+    from .. import external_typing as ext
+    from ..models import ModelStore
+
+    CatBoostModel = t.Union[


great refactoring 👍

bentoml/_internal/frameworks/catboost.py

parano · 2022-02-21T07:15:37Z

bentoml/_internal/frameworks/catboost.py

@@ -95,9 +89,7 @@ def load(
    tag: t.Union[str, Tag],
    model_params: t.Optional[t.Dict[str, t.Union[str, int]]] = None,
    model_store: "ModelStore" = Provide[BentoMLContainer.model_store],


I saw that we removed model_store inject from load_runner, should we also do that for save and load?

I think it makes more sense to just use public API under bentoml.models in our framework modules - I don't think allowing user to customize model_store for each save or load call is a valid use case, given that 1.0 will bring in a pretty powerful export/import feature that @sauyon is working on.

Let's do that in another PR

parano · 2022-02-21T07:18:17Z

bentoml/_internal/frameworks/common/model_runner.py

+
+    @property
+    def default_name(self) -> str:
+        return Tag.from_taglike(self._tag).name


any reason not to do self._tag = Tag.from_taglike(tag) in __init__?

parano · 2022-02-21T07:18:40Z

bentoml/_internal/frameworks/common/model_runner.py

+        name: t.Optional[str] = None,
+    ):
+        super().__init__(name)
+        self._tag = tag


bentoml/_internal/runner/runner.py

parano · 2022-02-21T07:22:35Z

bentoml/_internal/runner/runner.py

    def run(self, *args: t.Any, **kwargs: t.Any) -> t.Any:
        return self._impl.run(*args, **kwargs)

+    @final


parano · 2022-02-21T07:31:09Z

bentoml/_internal/runner/runner.py

    INIT = 0
    SETTING = 1
    SET = 2
+    SHUTIING_DOWN = 3
+    SHUTDOWN = 4


Not related in this PR, suggest renaming the two states above:

UNINITILIZED = 0 INITIALIZING = 1 READY = 2

And should it also have an ERROR state? in case _setup failed in runner_app?

The names reflect our setup and shutdown API.

SET -> READY

* refactor(sdk): runner * refactor(sdk): simpler runner creation * refactor(frameworks): many fixes * fix resources_quota * fi * fix for <py38 * fix runner * fix it * remove model_store in integration tests * remove runner args * mod default model_store for integration tests * try fix keras * fix picklable_model tests * fix detectron * review * fix picklable_model * append Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>

* refactor(sdk): runner * refactor(sdk): simpler runner creation * refactor(frameworks): many fixes * fix resources_quota * fi * fix for <py38 * fix runner * fix it * remove model_store in integration tests * remove runner args * mod default model_store for integration tests * try fix keras * fix picklable_model tests * fix detectron * review * fix picklable_model * append

parano reviewed Feb 18, 2022

View reviewed changes

aarnphm mentioned this pull request Feb 19, 2022

chore(transformers): returns pipelines automatically #2292

Merged

5 tasks

parano reviewed Feb 21, 2022

View reviewed changes

bentoml/_internal/runner/runner.py Show resolved Hide resolved

parano reviewed Feb 21, 2022

View reviewed changes

bojiang added 8 commits February 22, 2022 15:58

refactor(sdk): runner

16a1ecc

refactor(sdk): simpler runner creation

587b4d6

refactor(frameworks): many fixes

b41c9b0

fix resources_quota

45b2f02

fi

017f542

fix for <py38

eb73cae

fix runner

b9cf66a

fix it

581544d

bojiang force-pushed the fix branch from fc979c9 to 581544d Compare February 22, 2022 07:58

bojiang added 9 commits February 23, 2022 10:53

remove model_store in integration tests

5161672

remove runner args

33eb659

mod default model_store for integration tests

7744b92

try fix keras

6e8bdeb

fix picklable_model tests

c094664

fix detectron

1d8432e

review

f4439db

fix picklable_model

2dc2318

append

0f8420b

bojiang changed the title ~~refactor(sdk&framework): framework runners~~ refactor(sdk&framework): [Broken] framework runners Feb 23, 2022

bojiang merged commit d95f427 into bentoml:main Feb 23, 2022

bojiang deleted the fix branch February 23, 2022 04:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(sdk&framework): [Broken] framework runners #2288

refactor(sdk&framework): [Broken] framework runners #2288

bojiang commented Feb 18, 2022

codecov bot commented Feb 18, 2022 •

edited

Loading

aarnphm commented Feb 18, 2022

parano Feb 18, 2022

parano Feb 18, 2022

parano Feb 18, 2022

parano Feb 21, 2022

parano Feb 21, 2022

parano Feb 21, 2022

bojiang Feb 23, 2022

parano Feb 21, 2022

parano Feb 21, 2022

parano Feb 21, 2022

parano Feb 21, 2022

bojiang Feb 23, 2022

bojiang Feb 23, 2022 •

edited

Loading

refactor(sdk&framework): [Broken] framework runners #2288

refactor(sdk&framework): [Broken] framework runners #2288

Conversation

bojiang commented Feb 18, 2022

Description

Motivation and Context

How Has This Been Tested?

Checklist:

codecov bot commented Feb 18, 2022 • edited Loading

Codecov Report

aarnphm commented Feb 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bojiang Feb 23, 2022 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Feb 18, 2022 •

edited

Loading

bojiang Feb 23, 2022 •

edited

Loading