[PR-1160]: Removed check for local usage#911
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request improves model configuration and initialization by making inference compute info requirements explicit and enabling ModelBuilder reuse across classes. The changes address a previous limitation where compute info was always required, which prevented local usage scenarios where this information is not needed.
Changes:
- Added
compute_info_requiredparameter to ModelBuilder to conditionally enforce inference compute info presence - Modified ModelServer and ModelRunLocally to accept an optional ModelBuilder instance, avoiding redundant instantiation
- Updated CLI commands to pass shared ModelBuilder instances to both ModelRunLocally and ModelServer
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
clarifai/runners/models/model_builder.py |
Added compute_info_required parameter to control when inference compute info is mandatory; updated _get_inference_compute_info to conditionally assert and default to empty dict |
clarifai/runners/server.py |
Added model_builder parameter to accept external ModelBuilder instance, avoiding redundant initialization |
clarifai/runners/models/model_run_locally.py |
Added model_builder parameter to accept external ModelBuilder instance for consistency |
clarifai/cli/model.py |
Updated to pass shared ModelBuilder instance to both ModelRunLocally and ModelServer |
tests/runners/test_model_upload.py |
Updated test to use compute_info_required=True when testing upload flow |
Comments suppressed due to low confidence (1)
clarifai/runners/server.py:154
- The docstring is missing documentation for the new
model_builderparameter. This parameter should be documented to explain its purpose and behavior, similar to howmodel_runner_localis documented.
"""Initialize the ModelServer.
Args:
model_path: Path to the model directory
model_runner_local: ModelRunLocally instance (This is used to run the model locally with different modes like container or environment and patch it to Local Runner)
"""
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot test fails with Traceback (most recent call last): |
|
@ackizilkale I've opened a new pull request, #946, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@copilot why did you open a PR for this? Why did not you make the commit right in here? |
|
@ackizilkale I've opened a new pull request, #947, to work on those changes. Once the pull request is ready, I'll request review from you. |
Replace deprecated descriptor.label with descriptor.is_repeated in serializers.py and protobuf.py to silence DeprecationWarnings. Fix test_local_runner_cli mocks to return a list of objects with .name attributes (matching actual MethodSignature protocol) instead of a plain dict, and update ModelServer assertion to include model_builder. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Minimum allowed line rate is |
ackizilkale
deleted the
PR-1160-inference-info-removal-for-local-runners
branch
This pull request introduces improvements to how model configuration and inference compute information are handled, making the requirements for compute info explicit and ensuring consistency when initializing model-related classes. The changes add a new
compute_info_requiredparameter toModelBuilder, update dependent classes to accept and use an externally providedModelBuilder, and enforce stricter validation when uploading models.Improvements to model configuration and initialization:
compute_info_requiredparameter to theModelBuilderclass and its initialization, allowing callers to specify if inference compute info is mandatory. The_get_inference_compute_infomethod now asserts the presence of this info when required and defaults to an empty dict if not present. [1] [2] [3] [4]upload_modelfunction and related tests to instantiateModelBuilderwithcompute_info_required=True, enforcing that inference compute info must be present in these contexts. [1] [2]Consistent usage of ModelBuilder across classes:
ModelRunLocallyandModelServerto accept an optionalModelBuilderinstance during initialization. If not provided, they create their own. This avoids redundant instantiation and ensures consistent configuration. [1] [2] [3]clarifai/cli/model.py) to pass the sameModelBuilderinstance to bothModelRunLocallyandModelServer, ensuring shared configuration and avoiding unnecessary reloading or validation. [1] [2]