Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 3, 2025

Summary

This PR fixes EAGLE-5341 by ensuring the model proto with secrets is loaded once during server initialization and is available for all predict requests.

Problem Statement

The server start servicer path was missing the model proto since it's only added to the ModelRunner. Request-type secrets were not being fetched in the GetModel call.

Solution

  • Update ModelBuilder.get_model_proto() to include additional_fields=["secrets"] in GetModelRequest
  • Update ModelServer.start_servicer() to pass model_proto to ModelServicer
  • Update ModelServicer.__init__() to accept and store model_proto parameter
  • Update ModelServicer methods to inject model_proto when request doesn't have it

Changes Made

1. ModelBuilder.get_model_proto() (clarifai/runners/models/model_builder.py)

# Add secrets to additional_fields to get request-type secrets
request.additional_fields.append("secrets")
  • Uses .append() since additional_fields is a repeated field in protobuf
  • Ensures request-type secrets are included in the model proto

2. ModelServicer (clarifai/runners/models/model_servicer.py)

def __init__(self, model, model_proto=None):
    self.model = model
    self.model_proto = model_proto
  • Added optional model_proto parameter for backward compatibility
  • Modified all three endpoints to inject model proto when not present:
    • PostModelOutputs: Single request injection
    • GenerateModelOutputs: Single request injection
    • StreamModelOutputs: Multiple request injection for streaming

3. ModelServer.start_servicer() (clarifai/runners/server.py)

self._servicer = ModelServicer(
    self._current_model, model_proto=self._builder.get_model_proto()
)
  • Passes model proto to servicer during initialization
  • Ensures servicer has same access to model configuration as runner

Benefits

✅ Model proto with secrets loaded once during server initialization
✅ Eliminates need to send model proto with every predict request
✅ Consistent behavior between ModelRunner and ModelServicer paths
✅ Better performance by avoiding redundant model proto fetches
✅ Request-type secrets properly included in model proto
✅ Maintains backward compatibility

Original prompt

EAGLE-5341 Load model proto once to stop expecting it from predict request

the server start servicer path is missing the model proto since it's only added to the ModelRunner. should we GetModel in the self._current_model so it's proto and model class are always together regardless of how they are served?

In the GetModel we use to fetch the model proto for the runner we need to include an additional_fields flag secrets so that we get the request tyoe secrets


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI changed the title [EAGLE-5341] Load model proto once to stop expecting it from predict request the server start servicer path is missing the model proto since it's only added to the ModelRunner. should we GetModel in the self._current_model so it's proto and model cla... Load model proto with secrets once at server initialization Nov 3, 2025
Copilot AI requested a review from wemoveon2 November 3, 2025 15:59
Copilot finished work on behalf of wemoveon2 November 3, 2025 15:59
@wemoveon2 wemoveon2 marked this pull request as ready for review November 3, 2025 16:10
@wemoveon2
Copy link
Contributor

@copilot I dont think we need the test

Copy link
Contributor Author

Copilot AI commented Nov 3, 2025

@copilot I dont think we need the test

Removed the test file in commit bbb4ea9.

Copilot finished work on behalf of wemoveon2 November 3, 2025 17:45
Copy link
Contributor Author

Copilot AI commented Nov 3, 2025

@copilot also address formatting errors

--- a/tests/runners/test_model_proto_injection.py
...

The test file shown in the diff was already removed in commit bbb4ea9 per your earlier feedback. The implementation files (model_builder.py, model_servicer.py, server.py) are properly formatted - verified with ruff format --check.

Copilot finished work on behalf of wemoveon2 November 3, 2025 18:08
@github-actions
Copy link

github-actions bot commented Nov 3, 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 59%
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% (8217 / 13338)

Minimum allowed line rate is 50%

@wemoveon2 wemoveon2 merged commit d49cf47 into master Nov 4, 2025
24 of 25 checks passed
@wemoveon2 wemoveon2 deleted the copilot/fix-8550654-20333051-52e6908a-f5da-4a7d-a127-14e93f6da774 branch November 4, 2025 19:27
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.

3 participants