Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 5, 2025

Why

  • Model proto caching and merging logic is no longer needed in the SDK
  • Requests will contain model proto directly when required, eliminating need for SDK-side caching
  • Simplifies the codebase by removing unnecessary complexity

How

  • Removed model_proto parameter from ModelRunner.__init__ and ModelServicer.__init__
  • Removed self.model_proto instance variable from both classes
  • Removed all model proto merging/copying logic from:
    • ModelRunner: runner_item_predict(), runner_item_generate(), pmo_iterator()
    • ModelServicer: PostModelOutputs(), GenerateModelOutputs(), StreamModelOutputs()
  • Removed resources_pb2 import from model_servicer.py (no longer needed)
  • Removed model_proto argument from ModelServicer and ModelRunner initialization in server.py
  • Deleted tests/runners/test_model_proto_merge.py (tested removed functionality)

Tests

  • Syntax validation passed
  • Linting passed (ruff)
  • Formatting verified
  • Code compiles successfully

Notes

  • Requests now handle model proto directly without SDK-side caching or merging
  • This simplifies the code and moves responsibility to the request layer where it belongs
  • No breaking changes for callers who don't use the removed model_proto parameter
Original prompt

In clarifai/runners/models/model_runner.py and clarifai/runners/models/model_servicer.py, we should merge the cached model proto with the model proto in the request instead of only doing so when the request is misisng the model proto

The request will contain the model and model version proto when the user provided request type secrets or overrides the param specs inference parameters. When it is included, it will pretty much be a minimal model and model version proto that contains the IDs and model_version.output_info which contains the inference params and the request type secret


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

- Modified model_runner.py to merge cached model_proto with request.model
- Modified model_servicer.py to merge cached model_proto with request.model
- Changed from conditional CopyFrom to unconditional MergeFrom pattern
- Added comprehensive unit tests in test_model_proto_merge.py
- All tests passing, linting clean

Co-authored-by: wemoveon2 <86081107+wemoveon2@users.noreply.github.com>
Copilot AI changed the title [WIP] Merge cached model proto with request model proto Merge cached model proto with request model proto unconditionally Nov 5, 2025
Copilot AI requested a review from wemoveon2 November 5, 2025 22:13
Copilot finished work on behalf of wemoveon2 November 5, 2025 22:13
@wemoveon2 wemoveon2 changed the title Merge cached model proto with request model proto unconditionally [EAGLE-5341] Merge cached model proto with request model proto Nov 6, 2025
@wemoveon2 wemoveon2 marked this pull request as ready for review November 6, 2025 19:47
@wemoveon2
Copy link
Contributor

@copilot why not something like

if self.model_proto is not None:
if not request.HasField("model"):
request.model.CopyFrom(self.model_proto)
else:
merged_model = resources_pb2.Model()
merged_model.CopyFrom(self.model_proto)
merged_model.MergeFrom(request.model)
request.model.CopyFrom(merged_model)

- Changed from always creating temp model to explicit if-else branches
- When request has no model: directly copy cached proto (unchanged)
- When request has model: create merged_model, copy cached, merge request
- More readable code that separates the two logical paths
- All tests passing, no functional changes

Co-authored-by: wemoveon2 <86081107+wemoveon2@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 6, 2025

@copilot why not something like

if self.model_proto is not None:
...

Good suggestion! Updated to use the explicit if-else pattern for better readability. The two code paths are now clearly separated:

  • No model in request → direct CopyFrom
  • Model in request → create merged_model, copy cached, merge request

Changes in commit ccea79e. All tests still passing.

Copilot finished work on behalf of wemoveon2 November 6, 2025 19:59
@wemoveon2
Copy link
Contributor

@copilot describe the behaviors tested in the tests you added

Copy link
Contributor Author

Copilot AI commented Nov 6, 2025

@copilot describe the behaviors tested in the tests you added

The tests validate two key scenarios across different code paths:

Scenario 1: Empty request (no model proto)

  • Tests verify cached model proto is directly copied to empty requests
  • Validates all cached fields (id, user_id, app_id) appear in request
  • Covered in: test_pmo_iterator_with_empty_request_model, test_servicer_post_model_outputs_with_empty_request

Scenario 2: Request with minimal model proto (the bug case)

  • Tests verify both cached AND request fields are preserved after merge
  • Request fields (id, version, output_info with inference params) override cached values
  • Cached fields (user_id, app_id) that aren't in request are added
  • Validates the core fix: inference params from request work alongside cached config
  • Covered in: test_pmo_iterator_with_request_model_override, test_servicer_post_model_outputs_with_request_override, test_servicer_generate_model_outputs_merge, test_servicer_stream_model_outputs_merge

Each test also verifies the operation succeeds (SUCCESS status code) and covers different API methods (predict, generate, stream).

Copilot finished work on behalf of wemoveon2 November 6, 2025 21:17
@wemoveon2 wemoveon2 requested a review from luv-bansal November 7, 2025 05:59
@wemoveon2
Copy link
Contributor

@copilot drop passing of model proto entirely. the request will contain a model proto if it needs it, does not need anything on the sdk side.

remove the usage of the cached model proto from both the model runner and grpc servicer

- Removed model_proto parameter from ModelRunner.__init__
- Removed model_proto parameter from ModelServicer.__init__
- Removed all model proto merging/copying logic from both classes
- Removed model_proto parameter from pmo_iterator function
- Removed resources_pb2 import from model_servicer.py (no longer needed)
- Deleted test_model_proto_merge.py test file
- Request will contain model proto directly when needed per requirements

Co-authored-by: wemoveon2 <86081107+wemoveon2@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 7, 2025

@copilot drop passing of model proto entirely. the request will contain a model proto if it needs it, does not need anything on the sdk side.

remove the usage of the cached model proto from both th...

Removed all model_proto caching logic as requested. Changes in commit 6730513:

  • Removed model_proto parameter from ModelRunner and ModelServicer
  • Removed all merging/copying logic in both runner and servicer
  • Removed model_proto parameter from pmo_iterator
  • Deleted test_model_proto_merge.py
  • Requests now handle model proto directly

Copilot AI changed the title [EAGLE-5341] Merge cached model proto with request model proto [EAGLE-5341] Remove model_proto caching from ModelRunner and ModelServicer Nov 7, 2025
Copilot finished work on behalf of wemoveon2 November 7, 2025 18:02
Copilot AI requested a review from wemoveon2 November 7, 2025 18:02
@wemoveon2
Copy link
Contributor

@copilot we need to stop passing the model proto in server.py

- Removed model_proto argument from ModelServicer initialization
- Removed model_proto argument from ModelRunner initialization
- Server no longer passes cached model proto to servicer or runner
- Aligns with approach that requests contain model proto directly

Co-authored-by: wemoveon2 <86081107+wemoveon2@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 7, 2025

@copilot we need to stop passing the model proto in server.py

Removed model_proto from server.py. Changes in commit 5ac9566:

  • Removed model_proto argument when creating ModelServicer
  • Removed model_proto argument when creating ModelRunner
  • Server no longer passes model proto to either component

Copilot AI changed the title [EAGLE-5341] Remove model_proto caching from ModelRunner and ModelServicer [EAGLE-5341] Remove model_proto caching from ModelRunner, ModelServicer, and server Nov 7, 2025
Copilot finished work on behalf of wemoveon2 November 7, 2025 18:07
@github-actions
Copy link

github-actions bot commented Nov 7, 2025

Code Coverage

Package Line Rate Health
clarifai 45%
clarifai.cli 44%
clarifai.cli.templates 33%
clarifai.client 67%
clarifai.client.auth 67%
clarifai.constants 100%
clarifai.datasets 100%
clarifai.datasets.export 80%
clarifai.datasets.upload 75%
clarifai.datasets.upload.loaders 37%
clarifai.models 100%
clarifai.modules 0%
clarifai.rag 72%
clarifai.runners 53%
clarifai.runners.models 61%
clarifai.runners.pipeline_steps 41%
clarifai.runners.pipelines 70%
clarifai.runners.utils 63%
clarifai.runners.utils.data_types 72%
clarifai.schema 100%
clarifai.urls 60%
clarifai.utils 60%
clarifai.utils.evaluation 67%
clarifai.workflows 95%
Summary 62% (8364 / 13480)

Minimum allowed line rate is 50%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants