-
Notifications
You must be signed in to change notification settings - Fork 8
Add optional protobuf response information in pythonic models with parameter validation #810
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: zeiler <2138258+zeiler@users.noreply.github.com>
Co-authored-by: zeiler <2138258+zeiler@users.noreply.github.com>
Co-authored-by: zeiler <2138258+zeiler@users.noreply.github.com>
clarifai/client/model_client.py
Outdated
def bind_f(method_name, method_argnames, call_func, async_call_func): | ||
def sync_f(*args, **kwargs): | ||
# Extract with_proto parameter if present | ||
with_proto = kwargs.pop('with_proto', False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we ad "with_proto" as a constant and then in ModelClass verify that a user provided method does not have that argument name when they subclass ModelClass. That vailidation should be in method_signatures.py.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added RESERVED_PARAM_WITH_PROTO constant in method_signatures.py and validation in build_function_signature() to prevent user methods from using this reserved parameter name. ModelClient now uses the constant instead of hardcoded strings. Commit: 3b8ff57
clarifai/client/model_client.py
Outdated
proto = resources_pb2.Input() | ||
serialize({stream_argname: item}, [stream_sig], proto.data) | ||
yield proto | ||
# subsequent items are just the stream items |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did this get indented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the indentation issue - the "subsequent items are just the stream items" comment and async for loop were incorrectly indented inside the _input_proto_stream function. They should be at the module level. Commit: 3b8ff57
@copilot please address comment.s |
Co-authored-by: zeiler <2138258+zeiler@users.noreply.github.com>
Minimum allowed line rate is |
Problem
When using pythonic models (subclasses of ModelClass), users could only access the processed/deserialized response from method calls like
predict()
,generate()
, etc. However, the underlying protobuf API response contains valuable additional information such as:Users requested the ability to optionally access this underlying protobuf response while maintaining backward compatibility.
Solution
This PR adds support for an optional
with_proto=True/False
parameter to all pythonic model methods. When enabled, methods return both the processed result and the raw protobuf response as a tuple.Key Features
with_proto
defaults toFalse
generate()
andstream()
with_proto
parameter nameUsage Examples
Parameter Name Protection
The framework now validates that user methods cannot use the reserved parameter name:
Implementation Details
Core Changes
bind_f()
to extract and handle thewith_proto
parameter before argument validation_predict
,_generate
,_stream
methods (both sync and async versions) now acceptwith_proto
parameterwith_proto=True
, methods return(result, proto_response)
tuple; otherwise return just the resultRESERVED_PARAM_WITH_PROTO
constant and validation inbuild_function_signature()
to prevent conflictsFiles Modified
clarifai/client/model_client.py
: Core implementation and constant usageclarifai/runners/utils/method_signatures.py
: Parameter validation and constant definitiontests/test_with_proto_feature.py
: Comprehensive test suite (14 tests)examples/with_proto_demo.py
: Usage examples and documentationBackward Compatibility
The implementation is fully backward compatible:
with_proto=False
) unchangedTesting & Quality
This enhancement provides powerful debugging and inspection capabilities while maintaining the clean, pythonic interface that users expect and preventing parameter name conflicts through proactive validation.
Original prompt
<issue_description>When calling a pythonic model that is a sublcass of ModelClass we allow using the model_client.py code to call the function directly. So if the subclass of ModelClass has a method predict(prompt, reasoning_effort) then the client side can do:
We would like to allow any of those methods to accept an additional parameter like
with_proto=True/False
that would change the client side function call to return two pieces of information: 1) the current response as it does today and 2) additional details we have from the underlying protobuf by returning the underlying proto that the API returns.</issue_description>Comments on the Issue (you are @copilot in this section)
Fixes #809
Original prompt
Fixes #809
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.