Skip to content

Fix Minor local-runner issue#999

Merged
luv-bansal merged 2 commits intomasterfrom
fix-serve-issue
Mar 24, 2026
Merged

Fix Minor local-runner issue#999
luv-bansal merged 2 commits intomasterfrom
fix-serve-issue

Conversation

@luv-bansal
Copy link
Contributor

@luv-bansal luv-bansal commented Mar 24, 2026

Pull request overview

This PR updates the clarifai model serve (local runner) flow to ensure the target app is PUBLIC when reusing an existing app, which is needed for serving public models without failing due to app visibility.

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 updates the clarifai model serve (local runner) flow to ensure the target app is PUBLIC when reusing an existing app, which is needed for serving public models without failing due to app visibility.

Changes:

  • Import service_pb2 alongside resources_pb2 for additional gRPC request types.
  • When the app already exists, issue a patch request to set app visibility to PUBLIC before proceeding.

Comment on lines +2033 to +2038
patch_req = service_pb2.PatchAppsRequest(
user_app_id=user.user_app_id,
apps=[resources_pb2.App(id=app_id, visibility=public_visibility)],
action='overwrite',
)
user._grpc_request(user.STUB.PatchApps, patch_req)
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

This code manually constructs a PatchAppsRequest and calls user._grpc_request(user.STUB.PatchApps, ...), which ties the CLI to low-level gRPC details and private APIs. Since the SDK already provides User.patch_app(...), prefer calling that helper to avoid depending on the presence/signature of STUB.PatchApps and to keep request construction consistent with the rest of the client.

Suggested change
patch_req = service_pb2.PatchAppsRequest(
user_app_id=user.user_app_id,
apps=[resources_pb2.App(id=app_id, visibility=public_visibility)],
action='overwrite',
)
user._grpc_request(user.STUB.PatchApps, patch_req)
user.patch_app(app_id=app_id, visibility=public_visibility)

Copilot uses AI. Check for mistakes.
apps=[resources_pb2.App(id=app_id, visibility=public_visibility)],
action='overwrite',
)
user._grpc_request(user.STUB.PatchApps, patch_req)
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

New behavior is introduced here (patching an existing app to PUBLIC visibility) but the existing clarifai model serve CLI tests don’t assert that any patch call is made when resources already exist. Consider adding/adjusting the serve CLI unit tests to verify the app-visibility patch is invoked (and that failures are handled) to prevent regressions.

Suggested change
user._grpc_request(user.STUB.PatchApps, patch_req)
try:
user._grpc_request(user.STUB.PatchApps, patch_req)
except Exception as exc:
logger.error(
"Failed to update visibility to PUBLIC for app '%s': %s",
app_id,
exc,
)
raise UserError(
"Failed to update existing app visibility to PUBLIC. "
"Please verify your permissions and try again."
) from exc

Copilot uses AI. Check for mistakes.
@luv-bansal luv-bansal requested a review from a team March 24, 2026 07:47
@github-actions
Copy link

Code Coverage

Package Line Rate Health
clarifai 45%
clarifai.cli 62%
clarifai.cli.templates 67%
clarifai.cli.templates.toolkits 100%
clarifai.client 65%
clarifai.client.auth 67%
clarifai.constants 100%
clarifai.datasets 100%
clarifai.datasets.export 69%
clarifai.datasets.upload 75%
clarifai.datasets.upload.loaders 37%
clarifai.models 100%
clarifai.rag 0%
clarifai.runners 52%
clarifai.runners.models 58%
clarifai.runners.pipeline_steps 39%
clarifai.runners.pipelines 72%
clarifai.runners.utils 62%
clarifai.runners.utils.data_types 72%
clarifai.schema 100%
clarifai.urls 58%
clarifai.utils 65%
clarifai.utils.evaluation 16%
clarifai.workflows 95%
Summary 60% (11460 / 19090)

Minimum allowed line rate is 50%

@brajrajnagar brajrajnagar self-requested a review March 24, 2026 08:21
@luv-bansal luv-bansal merged commit 9400c86 into master Mar 24, 2026
21 of 23 checks passed
@luv-bansal luv-bansal deleted the fix-serve-issue branch March 24, 2026 08:30
@ackizilkale ackizilkale mentioned this pull request Mar 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants