[PR-674] fix legacy proto support #636
Conversation
| for name in ['predict']: # , 'generate', 'stream'): | ||
| if hasattr(cls, name): | ||
| method = getattr(cls, name) | ||
| if name not in methods: # not already put in registry | ||
| methods[name] = _MethodInfo(method, proto_method=True) |
There was a problem hiding this comment.
I think this only checks for a class attribute — not necessarily a method. And if it is a method, it ensures it's not decorated with @ModelClass.method and is named predict. I don't think it ensures a method is in old proto-style method that returns a proto.
| for name in ['predict']: # , 'generate', 'stream'): | ||
| if hasattr(cls, name): | ||
| method = getattr(cls, name) | ||
| if name not in methods: # not already put in registry | ||
| methods[name] = _MethodInfo(method, proto_method=True) |
There was a problem hiding this comment.
@zeiler I think this only checks for a class attribute — not necessarily a method. And if it is a method, it ensures it's not decorated with @ModelClass.method and is named predict. I don't think it ensures a method is in old proto-style method that returns a proto.
There was a problem hiding this comment.
Fixed in my follow-up commit
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR enhances support for legacy proto-style model methods by introducing detection and special handling, renaming the method-info API, and updating tests and utilities accordingly.
- Add
is_proto_style_methodand integrate proto-method handling inModelClass - Rename
_get_method_info→_get_method_infosacross code, tests, and builder scripts - Introduce new runner integration tests and a dummy proto-based runner model
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/runners/test_runners_proto.py | New end-to-end tests for the proto runner server |
| tests/runners/test_model_signatures.py | Updated signature tests to use _get_method_infos() |
| tests/runners/dummy_runner_proto_model/requirements.txt | Added dependencies for dummy runner model |
| tests/runners/dummy_runner_proto_model/config.yaml | Configuration for dummy runner proto model |
| tests/runners/dummy_runner_proto_model/1/model.py | Dummy runner implementation handling text and image inputs |
| setup.py | Exclude ruff from install_requires |
| clarifai/runners/utils/model_utils.py | Added is_proto_style_method helper |
| clarifai/runners/utils/code_script.py | Guard against None method signatures in script gen |
| clarifai/runners/models/model_class.py | Rename API, register proto-style methods, use logger |
| clarifai/runners/models/model_builder.py | Updated builder to call _get_method_infos() |
Comments suppressed due to low confidence (1)
clarifai/runners/utils/model_utils.py:140
- There are no unit tests covering the new
is_proto_style_methodfunction. Adding tests for its various signature patterns would help prevent regressions.
def is_proto_style_method(method):
| if not proto_method: | ||
| self.signature = build_function_signature(method) | ||
| else: | ||
| self.signature = None | ||
| self.python_param_types = { |
There was a problem hiding this comment.
The python_param_types dict currently includes all parameters (including self) and unannotated ones. You should filter out 'self' and only include parameters with explicit annotations, as in the previous implementation (if p.annotation != inspect.Parameter.empty and then pop('self', None)).
Copilot uses AI. Check for mistakes.
Minimum allowed line rate is |
luv-bansal
changed the title
| return any(method_signature.name == name for method_signature in method_signatures) | ||
| return any( | ||
| method_signature.name == name for method_signature in method_signatures if method_signature | ||
| ) | ||
|
|
||
|
|
||
| def generate_client_script( |
There was a problem hiding this comment.
i never implemented what the API call is in the code snippet, that would be the only thing missing showing how to use the client properly to call this proto method. Most of the SDK functions like model.predict() I believe are still in this format so should work?
| # if not hasattr(method, _METHOD_INFO_ATTR): # not already put in registry | ||
| # methods[name] = _MethodInfo(method) | ||
| # older models never had generate or stream so don't bother with them. | ||
| for name in ['predict']: # , 'generate', 'stream'): |
There was a problem hiding this comment.
wonder if we should use the word "PostModelOutputs" to match the grpc call with the method name? Then it would be something very unlikely for a user to use as a function name when implementing their own model while being more clear for us that this is the proto handler? If we change it there is another hard coded "predict" above in this code that matches this method name.
Pull Request Overview
This PR enhances support for legacy proto-style model methods by introducing detection and special handling, along with adding tests and required configuration changes.
is_proto_style_methodand integrate proto-method handling inModelClass_get_method_info→_get_method_infosacross code, tests, and builder scripts