Conversation
There was a problem hiding this comment.
Pull request overview
This PR changes the default visibility of resources created/used by the “local runner” flow so they are PUBLIC by default (intended to make locally served models more easily shareable/accessible through Clarifai).
Changes:
- Set default local-runner compute cluster and nodepool configs to PUBLIC visibility.
- Update
clarifai model serveto create the app, model, model version, and deployment with PUBLIC visibility.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
clarifai/utils/constants.py |
Adds PUBLIC visibility defaults to local runner compute cluster/nodepool config dicts. |
clarifai/cli/model.py |
Applies PUBLIC visibility when creating local runner app/model/version and sets deployment visibility to PUBLIC. |
| "visibility": { | ||
| "gettable": 50, # PUBLIC | ||
| }, |
There was a problem hiding this comment.
Avoid hard-coding the enum value 50 for PUBLIC visibility. Consider using resources_pb2.Visibility.Gettable.PUBLIC (or a named constant) here as well, for consistency and to reduce the risk of enum value drift.
| version_model = model.create_version( | ||
| pretrained_model_config={"local_dev": True}, | ||
| method_signatures=method_signatures, | ||
| visibility=public_visibility, | ||
| ) |
There was a problem hiding this comment.
There are existing tests for clarifai model serve (see tests/cli/test_local_runner_cli.py), but they don’t currently assert that resources are created with PUBLIC visibility. Please add assertions that create_app, create_model, create_version, and create_deployment are called with the expected visibility to prevent regressions.
| "visibility": { | ||
| "gettable": resources_pb2.Visibility.Gettable.PUBLIC, | ||
| }, |
There was a problem hiding this comment.
Making the deployment PUBLIC by default is a high-impact security change. Consider making this configurable (flag/config), and/or printing the final shared URL with an explicit warning so users understand the exposure they’re enabling.
| "visibility": { | ||
| "gettable": 50, # PUBLIC | ||
| }, |
There was a problem hiding this comment.
Setting the local runner compute cluster visibility to PUBLIC changes who can discover/read this resource. If the intent is only to make the served model shareable, consider keeping infra resources (compute cluster/nodepool) private by default, or making this configurable via a CLI flag / config option.
| "min_instances": 1, | ||
| "max_instances": 1, | ||
| "visibility": { | ||
| "gettable": 50, # PUBLIC |
There was a problem hiding this comment.
Making the default local runner nodepool PUBLIC can expose resource metadata to other users depending on platform ACL semantics. If public sharing is only needed for prediction, consider leaving the nodepool private by default and only making the app/model/version/deployment public (or make this behavior opt-in).
| "gettable": 50, # PUBLIC | |
| "gettable": 10, # PRIVATE |
| # Public visibility so anyone with the URL can send predictions | ||
| public_visibility = resources_pb2.Visibility( | ||
| gettable=resources_pb2.Visibility.Gettable.PUBLIC | ||
| ) | ||
| try: | ||
| app = user.app(app_id) | ||
| out.status("App ready") | ||
| except Exception: | ||
| out.status("Creating app... ", nl=False) | ||
| app = user.create_app(app_id) | ||
| app = user.create_app(app_id, visibility=public_visibility) | ||
| click.echo("done") |
There was a problem hiding this comment.
Defaulting the local runner’s app/model/version visibility to PUBLIC is a security-sensitive change (it can unintentionally expose models to broader audiences). Consider requiring an explicit opt-in flag (e.g., --public), or at least printing a prominent warning + providing a --private override to avoid accidental exposure.
| try: | ||
| app = user.app(app_id) | ||
| out.status("App ready") | ||
| except Exception: | ||
| out.status("Creating app... ", nl=False) | ||
| app = user.create_app(app_id) | ||
| app = user.create_app(app_id, visibility=public_visibility) | ||
| click.echo("done") |
There was a problem hiding this comment.
Visibility is only applied when creating the app. If the app already exists (possibly created by an older version of this command with private visibility), this path won’t update it to PUBLIC, so “public by default” may not take effect for existing users. Consider checking/patching existing app visibility (e.g., via User.patch_app) or warning when it’s not public.
| model = app.create_model( | ||
| model_id, model_type_id=model_type_id, visibility=public_visibility | ||
| ) |
There was a problem hiding this comment.
Visibility is only applied when creating the model. If an existing model is found, its visibility is left unchanged, which can conflict with the new “public by default” behavior. Consider checking/patching existing model visibility (e.g., App.patch_model) or at least emitting a warning when it’s not public.
| ], | ||
| "deploy_latest_version": True, | ||
| "visibility": { | ||
| "gettable": resources_pb2.Visibility.Gettable.PUBLIC, |
There was a problem hiding this comment.
public_visibility is created as a resources_pb2.Visibility proto, but deployment visibility is set via a separate inline dict and repeats the enum reference. Consider reusing the already-defined value (e.g., use public_visibility.gettable) to keep visibility handling consistent and reduce duplication.
| "gettable": resources_pb2.Visibility.Gettable.PUBLIC, | |
| "gettable": public_visibility.gettable, |
| "visibility": { | ||
| "gettable": 50, # PUBLIC | ||
| }, |
There was a problem hiding this comment.
Avoid hard-coding the enum value 50 for PUBLIC visibility. Since this dict is later converted to resources_pb2.Visibility(**...), consider using resources_pb2.Visibility.Gettable.PUBLIC (or a named constant in this module) to prevent silent breakage if enum values ever change and to improve readability.
|
All good, but make sure to write content into PR description |
Minimum allowed line rate is |
Pull request overview
This PR changes the default visibility of resources created/used by the “local runner” flow so they are PUBLIC by default (intended to make locally served models more easily shareable/accessible through Clarifai).