multiple requests according to model type #544
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
abhash-ai
reviewed
Apr 1, 2025
Comment on lines
+103
to
+106
| default_request = service_pb2.PostModelOutputsRequest( | ||
| model=resources_pb2.Model(model_version=model_version_proto), | ||
| inputs=[ | ||
| resources_pb2.Input( |
There was a problem hiding this comment.
Dont need a default request, can just use _build_text_to_text_requests as default request no?
abhash-ai
reviewed
Apr 1, 2025
Comment on lines
+130
to
+132
| def _build_text_to_text_requests(): | ||
| requests = [ | ||
| service_pb2.PostModelOutputsRequest( |
There was a problem hiding this comment.
Nit: I think we can reduce duplication by creating a function: CreateRequest(model_version_proto, image_input, text_input), where you can pass in the Image, Text and ModelVersionProto as params?
Would make it reusable elsewhere too
abhash-ai
reviewed
Apr 1, 2025
Comment on lines
+254
to
+258
| except Exception as e: | ||
| logger.error(f"Model Generation failed: {e}") | ||
| traceback.print_exc() | ||
| generate_response = service_pb2.MultiOutputResponse( | ||
| status=status_pb2.Status( |
There was a problem hiding this comment.
Unrelated to this PR, but shouldn't the predict / generate stream functions themselves perform this fallback to calling MultiOutputResponse API?
abhash-ai
reviewed
Apr 1, 2025
Comment on lines
+365
to
+367
| command = [ | ||
| self.python_executable, | ||
| "-m", |
abhash-ai
reviewed
Apr 1, 2025
Comment on lines
+447
to
+448
| [ | ||
| "docker", |
There was a problem hiding this comment.
Same as another one, single like was better no?
luv-bansal
reviewed
Apr 2, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ticket1: https://clarifai.atlassian.net/browse/PR-468
Ticket2: https://clarifai.atlassian.net/browse/PR-471
For multimodal, the image and the text in the test is unrelated. Made them related to make sure the image is taken into consideration by the model
Only image url seems to be considered. Enabled passing imagebytes too, as some models do image processing before prediction
Also added test for multimodal models with only text in the input
Logged the last token in the streamed response