add predict_fn_or_cls as a bundle - #246
Conversation
| @@ -103,6 +107,7 @@ def create_model_bundle( | |||
| model: Typically a trained Neural Network, e.g. a Pytorch module | |||
| load_model_fn: Function that when run, loads a model, e.g. a Pytorch module | |||
| load_predict_fn: Function that when called with model, returns a function that carries out inference | |||
There was a problem hiding this comment.
It looks like the original type annotation of load_predict_fn was a Callable: Callable[[DeployModel_T], Callable[[Any], Any]]. In that case, maybe we don't need to create a new predict_fn_or_cls field, since load_predict_fn was already forwards compatible with the way you've specified the Bundle class with a __call__ method?
There was a problem hiding this comment.
They have different usage:
a. predict_fn_or_cls
foo("woo") # returns inference result;
client.upload_bundle(predict_fn_or_cls=foo)
b. load_predict_fn
foo("woo") # returns inference result;
# incorrect:
client.upload_bundle(load_predict_fn=foo)
# correct:
def load_predict_fn(model):
def predict(args):
return foo(args)
return predict
There was a problem hiding this comment.
I mean predict_fn_or_cls is a function/class that you can use directly in the service. load_predict_fn on the other hand is a factory that produces a callable object.
There was a problem hiding this comment.
Oh I see, the absence or presence of the load prefix can serve as a distinction as to whether or not it's directly used or not 👌🏻
| """ | ||
| Grabs a s3 signed url and uploads a model bundle to Scale Deploy. | ||
| A model bundle consists of a "load_predict_fn" and exactly one of "model" or "load_model_fn", such that | ||
| A model bundle consists of a "predict_fn_or_cls" or "load_predict_fn" and exactly one of "model" or "load_model_fn", such that |
There was a problem hiding this comment.
I think the allowed combinations are {predict_fn_or_cls}, {load_predict_fn + model}, {load_predict_fn + load_model_fn}, I'm not sure if it's completely clear from reading this. Maybe something like "A model bundle consists of {predict_fn_or_cls}, {load_predict_fn + model}, or {load_predict_fn + load_model_fn}.".
| if sum(check_args) != 1: | ||
| raise ValueError( | ||
| "Exactly one of model and load_model_fn should be non-None" | ||
| "Exactly one of `model` or `load_model_fn` or `predict_fn_or_cls` should be non-None" |
There was a problem hiding this comment.
Should we check that load_predict_fn doesn't get passed if a user passes predict_fn_or_cls?
| load_predict_fn: Optional[ | ||
| Callable[[DeployModel_T], Callable[[Any], Any]] | ||
| ] = None, | ||
| predict_fn_or_cls: Optional[Callable[[Any], Any]] = None, |
There was a problem hiding this comment.
Overall I think this is fine for now, but we might as well start deprecating load_predict_fn later.
There was a problem hiding this comment.
Oh I guess same comment as https://github.com/scaleapi/models/pull/2684/files#r820019055, basically.
| ) | ||
| else: | ||
| bundle = dict( | ||
| bundle_func_2 = dict( |
There was a problem hiding this comment.
Can we just call both of these bundle_func? These branches are mutually exclusive, so even Python's funky dynamic scoping rules wouldn't be an issue.
There was a problem hiding this comment.
yeah, I did it for typing check that a linter performs. It complains that a variable bundle has different types in different branches, i.e dict/func/class_obj
I will play with the Optional[Dict...]
3 participants
Comes with https://github.com/scaleapi/models/pull/2684