Skip to content

[EAGLE-5341] Load model proto once to stop expecting it from predict request #822

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.

Sign up for GitHub

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

Jump to bottom
Merged
wemoveon2 merged 12 commits into from
Oct 23, 2025
Merged

[EAGLE-5341] Load model proto once to stop expecting it from predict request #822

wemoveon2 merged 12 commits into from
Oct 23, 2025

Conversation

@wemoveon2
Copy link
Contributor

@wemoveon2 wemoveon2 commented Oct 22, 2025 •
edited
Loading

What

  • Currently, the API sends the full model proto as part of each predict request

Why

  • This is unnecessary as we can load the model proto once and reuse it across all requests without depending on the API including it

How

  • Retrieve the model proto once via GetModel and set it in each predict request incase it's needed downstream

Tests

  • Existing tests pass

Notes

Copilot AI reviewed Oct 22, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR optimizes model runner performance by loading the model proto once at initialization instead of expecting it with every predict request from the API. The model proto is now retrieved via a single GetModel call and reused across all subsequent requests.

Key Changes

  • Added get_model_proto() method to ModelBuilder to fetch model metadata once via GetModel API
  • Updated ModelRunner to accept and store model_proto during initialization
  • Modified predict, generate, and stream methods to inject the cached model_proto into requests

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
clarifai/runners/models/model_builder.py Adds get_model_proto() method to retrieve and return model proto via GetModel API call
clarifai/runners/server.py Passes the retrieved model_proto to ModelRunner during initialization
clarifai/runners/models/model_runner.py Updates constructor to accept model_proto parameter and injects it into all request types (predict, generate, stream)

@wemoveon2
Update clarifai/runners/models/model_runner.py
f603dff 
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@wemoveon2 wemoveon2 requested a review from Copilot October 22, 2025 20:51
Copilot AI reviewed Oct 22, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

@wemoveon2 wemoveon2 requested a review from Copilot October 22, 2025 21:01
Copilot AI reviewed Oct 22, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

wemoveon2
wemoveon2 commented Oct 22, 2025
View reviewed changes
clarifai/runners/models/model_runner.py
Copy link
Contributor Author

@wemoveon2 wemoveon2 Oct 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue with this is we'll overwrite request type secrets if we don't also add handling for request type secrets to GetModel.

we'll need to ensure that we're populating the value for request type secrets in GetModel endpoint

@wemoveon2 wemoveon2 requested a review from Copilot October 22, 2025 21:14
Copilot AI reviewed Oct 22, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

wemoveon2 and others added 2 commits October 22, 2025 17:15
@wemoveon2
suggestion
73da291
@wemoveon2
Update clarifai/runners/models/model_runner.py
5e8dd7a 
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@wemoveon2 wemoveon2 requested a review from Copilot October 22, 2025 21:16
Copilot AI reviewed Oct 22, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

@wemoveon2 wemoveon2 requested a review from Copilot October 23, 2025 05:23
Copilot AI reviewed Oct 23, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

wemoveon2 and others added 3 commits October 23, 2025 01:24
@wemoveon2
Update clarifai/runners/models/model_runner.py
efef3cb 
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@wemoveon2
Update clarifai/runners/models/model_builder.py
26f11cc 
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@wemoveon2
fix test
f25ff9c
@github-actions
Copy link

github-actions bot commented Oct 23, 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 52% ➖
clarifai.runners.models 58% ➖
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 61% (8166 / 13290) ➖

Minimum allowed line rate is 50%

luv-bansal
luv-bansal approved these changes Oct 23, 2025
View reviewed changes
Copy link
Contributor

@luv-bansal luv-bansal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@wemoveon2 wemoveon2 merged commit bf25e09 into master Oct 23, 2025
11 checks passed
@wemoveon2 wemoveon2 deleted the 5341 branch October 23, 2025 15:42
srikanthbachala20 pushed a commit that referenced this pull request Oct 24, 2025
@wemoveon2 @srikanthbachala20
[EAGLE-5341] Load model proto once to stop expecting it from predict …
59b1716 
…request (#822)

* load model proto from GetModel request and set it for every predict request

* Update clarifai/runners/models/model_runner.py

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Revert "Update clarifai/runners/models/model_runner.py"

This reverts commit f603dff.

* fix suggestion

* only set if none

* suggestion

* Update clarifai/runners/models/model_runner.py

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* fix condition

* fix tests

* Update clarifai/runners/models/model_runner.py

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update clarifai/runners/models/model_builder.py

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* fix test

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
zeiler
zeiler reviewed Oct 25, 2025
View reviewed changes
clarifai/runners/server.py

def start_servicer(self, port, pool_size, max_queue_size, max_msg_length, enable_tls):
# initialize the servicer with the runner so that it gets the predict(), generate(), stream() classes.
self._servicer = ModelServicer(self._current_model)
Copy link
Member

@zeiler zeiler Oct 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this path missing the model proto since it's only added to the ModelRunner. How come we wouldn't GetModel in the self._current_model so it's proto and model class are always together regardless of how they are served. @luv-bansal @wemoveon2

@srikanthbachala20 srikanthbachala20 mentioned this pull request Nov 11, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zeiler zeiler zeiler left review comments

Copilot code review Copilot Copilot left review comments

@luv-bansal luv-bansal luv-bansal approved these changes

@ackizilkale ackizilkale Awaiting requested review from ackizilkale

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@wemoveon2 @zeiler @luv-bansal