From 1f0207d9bbd496aec120bcacbe5107f5d9ee593a Mon Sep 17 00:00:00 2001 From: sharang chopra Date: Mon, 25 May 2026 07:34:58 +0530 Subject: [PATCH 1/4] Make 'artifact list' default to user-in-context with flag interface Refactor 'clarifai artifact list' so it no longer requires a positional URI path. Adds --user_id/--app_id/--artifact_id flags matching the older flag convention (aligned in the Apr 30 UX Weekly). - --user_id defaults to the current CLI context user. - With no --app_id, list iterates the user's apps and aggregates artifacts (the backend ListArtifacts endpoint is app-scoped). - --versions now uses --app_id/--artifact_id flags instead of a URI. - Legacy positional URI path still works (with a deprecation warning) for back-compat. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- clarifai/cli/artifact.py | 168 ++++++++++++++++++++++++++------- tests/cli/test_artifact_cli.py | 122 +++++++++++++++++++++++- 2 files changed, 256 insertions(+), 34 deletions(-) diff --git a/clarifai/cli/artifact.py b/clarifai/cli/artifact.py index 56132aea..f53663e3 100644 --- a/clarifai/cli/artifact.py +++ b/clarifai/cli/artifact.py @@ -263,15 +263,41 @@ def artifact(): @artifact.command(['list', 'ls']) -@click.argument('path') -@click.option('--versions', is_flag=True, help='List artifact versions instead of artifacts') +@click.argument('path', required=False) +@click.option('--user_id', required=False, help='User ID. Defaults to the current context user.') +@click.option( + '--app_id', + required=False, + help="App ID. If omitted, lists artifacts across all of the user's apps.", +) +@click.option('--artifact_id', required=False, help='Artifact ID (required with --versions).') +@click.option('--versions', is_flag=True, help='List artifact versions instead of artifacts.') +@click.option('--page_no', type=int, default=None, help='Page number to list.') +@click.option('--per_page', type=int, default=None, help='Number of items per page.') @click.pass_context -def list(ctx, path, versions): +def list(ctx, path, user_id, app_id, artifact_id, versions, page_no, per_page): """List artifacts or artifact versions. + \b Examples: - clarifai af list users/u/apps/a - clarifai af list users/u/apps/a/artifacts/my-artifact --versions + # List all artifacts for the current user across all of their apps + clarifai af list + + \b + # Scope to a single app + clarifai af list --app_id my-app + + \b + # List versions of a specific artifact + clarifai af list --versions --app_id my-app --artifact_id my-artifact + + \b + # Another user + clarifai af list --user_id other-user --app_id their-app + + \b + # Legacy URI path (deprecated) + clarifai af list users/u/apps/a """ from clarifai_grpc.grpc.api import resources_pb2 @@ -282,25 +308,48 @@ def list(ctx, path, versions): try: validate_context(ctx) - # Parse path and extract components - if versions: + # Back-compat: parse legacy positional URI path and back-fill any unset flags. + if path: + logger.warning( + "Passing a URI path positionally is deprecated; use --user_id/--app_id/--artifact_id flags instead." + ) parsed = parse_artifact_path(path) - if not parsed['artifact_id']: + user_id = user_id or parsed.get('user_id') + app_id = app_id or parsed.get('app_id') + artifact_id = artifact_id or parsed.get('artifact_id') + + # Default user_id from the current CLI context. + if not user_id: + user_id = getattr(ctx.obj.current, 'user_id', None) + if not user_id: + click.echo( + "No user_id provided and none found in current context. " + "Pass --user_id or set a context with a user_id.", + err=True, + ) + raise click.Abort() + + if versions: + if not app_id or not artifact_id: click.echo( - "When using --versions, path must include artifact ID: users//apps//artifacts/", + "--versions requires --app_id and --artifact_id.", err=True, ) raise click.Abort() - # List artifact versions with full data artifact_version = ArtifactVersion( - artifact_id=parsed['artifact_id'], - user_id=parsed['user_id'], - app_id=parsed['app_id'], + artifact_id=artifact_id, + user_id=user_id, + app_id=app_id, pat=ctx.obj.current.pat, base=ctx.obj.current.api_base, ) - versions_list = builtin_list(artifact_version.list()) + list_kwargs = {} + if page_no is not None: + list_kwargs['page_no'] = page_no + if per_page is not None: + list_kwargs['per_page'] = per_page + versions_list = builtin_list(artifact_version.list(**list_kwargs)) if not versions_list: click.echo("No artifact versions found") @@ -321,28 +370,31 @@ def list(ctx, path, versions): 'CREATED_AT': lambda v: str(v.created_at.ToDatetime()) if v.created_at else '', }, ) - else: - # For listing artifacts, we expect app-level path: users//apps/ - if '/artifacts/' in path: - click.echo("To list artifacts, use: users//apps/", err=True) - raise click.Abort() - - # Parse app-level path - parsed = parse_artifact_path(path) - - # Validate it's an app-level path (no artifact_id) - if parsed['artifact_id'] is not None: - click.echo("To list artifacts, use: users//apps/", err=True) - raise click.Abort() + return - # List artifacts - artifact = Artifact( - user_id=parsed['user_id'], - app_id=parsed['app_id'], + # Listing artifacts (not versions). + if artifact_id: + click.echo( + "--artifact_id is only valid with --versions when listing.", + err=True, + ) + raise click.Abort() + + list_kwargs = {} + if page_no is not None: + list_kwargs['page'] = page_no + if per_page is not None: + list_kwargs['per_page'] = per_page + + if app_id: + # Single-app mode. + artifact_client = Artifact( + user_id=user_id, + app_id=app_id, pat=ctx.obj.current.pat, base=ctx.obj.current.api_base, ) - artifacts_list = builtin_list(artifact.list()) + artifacts_list = builtin_list(artifact_client.list(**list_kwargs)) if not artifacts_list: click.echo("No artifacts found") @@ -357,6 +409,58 @@ def list(ctx, path, versions): 'CREATED_AT': lambda a: str(a.created_at.ToDatetime()) if a.created_at else '', }, ) + return + + # User-default mode: iterate apps and aggregate artifacts. + from clarifai.client.user import User + + user_client = User( + user_id=user_id, + pat=ctx.obj.current.pat, + base_url=ctx.obj.current.api_base, + ) + apps = builtin_list(user_client.list_apps()) + if not apps: + click.echo("No apps found for user") + return + + logger.info( + f"Listing artifacts across {len(apps)} app(s) for user {user_id}. " + "Use --app_id to scope to a single app." + ) + + aggregated = [] + for app in apps: + app_id_value = getattr(app, 'id', None) or getattr(app, 'app_id', None) + if not app_id_value: + continue + try: + art_client = Artifact( + user_id=user_id, + app_id=app_id_value, + pat=ctx.obj.current.pat, + base=ctx.obj.current.api_base, + ) + for a in art_client.list(**list_kwargs): + aggregated.append((app_id_value, a)) + except Exception as inner: + logger.warning(f"Failed to list artifacts for app {app_id_value}: {inner}") + + if not aggregated: + click.echo("No artifacts found") + return + + app_id_by_artifact = {id(a): app_id_value for app_id_value, a in aggregated} + display_co_resources( + [a for _, a in aggregated], + custom_columns={ + 'APP_ID': lambda a: app_id_by_artifact.get(id(a), ''), + 'ARTIFACT': lambda a: a.id, + 'LATEST_VERSION': lambda a: _resolve_latest_version_id(a), + 'VISIBILITY': lambda a: _resolve_visibility(resources_pb2, a), + 'CREATED_AT': lambda a: str(a.created_at.ToDatetime()) if a.created_at else '', + }, + ) except UserError as e: click.echo(str(e), err=True) diff --git a/tests/cli/test_artifact_cli.py b/tests/cli/test_artifact_cli.py index 1621a951..7ad2c750 100644 --- a/tests/cli/test_artifact_cli.py +++ b/tests/cli/test_artifact_cli.py @@ -175,12 +175,130 @@ def test_list_command_latest_version_and_visibility( @patch('clarifai.cli.artifact.validate_context') def test_list_command_missing_params(self, mock_validate): - """Test list command with missing required parameters.""" + """Test list command with no user_id available errors clearly.""" mock_obj = setup_context_mock(mock_validate) + # Explicitly clear user_id on the context to simulate "no user in context". + mock_obj.current.user_id = None result = self.runner.invoke(artifact, ['list'], obj=mock_obj) assert result.exit_code != 0 - assert "Missing argument 'PATH'" in result.output + assert "No user_id provided" in result.output + + @patch('clarifai.cli.artifact.validate_context') + @patch('clarifai.client.artifact.Artifact') + def test_list_command_with_flags(self, mock_artifact_class, mock_validate): + """Test list command using --user_id/--app_id flag convention.""" + mock_validate.return_value = None + + mock_artifact_instance = Mock() + mock_artifact_instance.list.return_value = [] + mock_artifact_class.return_value = mock_artifact_instance + + mock_obj = create_mock_context() + + result = self.runner.invoke( + artifact, + ['list', '--user_id', 'test_user', '--app_id', 'test_app'], + obj=mock_obj, + ) + + if result.exit_code != 0: + print(f"Command failed with output: {result.output}") + + mock_artifact_class.assert_called_once() + kwargs = mock_artifact_class.call_args.kwargs + assert kwargs['user_id'] == 'test_user' + assert kwargs['app_id'] == 'test_app' + mock_artifact_instance.list.assert_called_once() + assert result.exit_code == 0 + + @patch('clarifai.cli.artifact.validate_context') + @patch('clarifai.client.user.User') + @patch('clarifai.client.artifact.Artifact') + def test_list_default_iterates_user_apps( + self, mock_artifact_class, mock_user_class, mock_validate + ): + """With no --app_id, list iterates the user's apps.""" + mock_validate.return_value = None + + app1 = Mock() + app1.id = 'app-1' + app2 = Mock() + app2.id = 'app-2' + mock_user_instance = Mock() + mock_user_instance.list_apps.return_value = iter([app1, app2]) + mock_user_class.return_value = mock_user_instance + + mock_artifact_instance = Mock() + mock_artifact_instance.list.return_value = [] + mock_artifact_class.return_value = mock_artifact_instance + + mock_obj = create_mock_context() + mock_obj.current.user_id = 'context_user' + + result = self.runner.invoke(artifact, ['list'], obj=mock_obj) + + if result.exit_code != 0: + print(f"Command failed with output: {result.output}") + + assert result.exit_code == 0 + mock_user_class.assert_called_once() + assert mock_user_class.call_args.kwargs.get('user_id') == 'context_user' + assert mock_artifact_class.call_count == 2 + app_ids_called = {c.kwargs['app_id'] for c in mock_artifact_class.call_args_list} + assert app_ids_called == {'app-1', 'app-2'} + + @patch('clarifai.cli.artifact.validate_context') + @patch('clarifai.client.artifact_version.ArtifactVersion') + def test_list_versions_with_flags(self, mock_artifact_version_class, mock_validate): + """List versions using --versions/--app_id/--artifact_id flags.""" + mock_validate.return_value = None + + mock_version_instance = Mock() + mock_version_instance.list.return_value = [] + mock_artifact_version_class.return_value = mock_version_instance + + mock_obj = create_mock_context() + + result = self.runner.invoke( + artifact, + [ + 'list', + '--versions', + '--user_id', + 'test_user', + '--app_id', + 'test_app', + '--artifact_id', + 'test_artifact', + ], + obj=mock_obj, + ) + + if result.exit_code != 0: + print(f"Command failed with output: {result.output}") + + mock_artifact_version_class.assert_called_once() + kwargs = mock_artifact_version_class.call_args.kwargs + assert kwargs['user_id'] == 'test_user' + assert kwargs['app_id'] == 'test_app' + assert kwargs['artifact_id'] == 'test_artifact' + assert result.exit_code == 0 + + @patch('clarifai.cli.artifact.validate_context') + def test_list_versions_requires_artifact_id(self, mock_validate): + """--versions without --artifact_id should error.""" + mock_validate.return_value = None + mock_obj = create_mock_context() + mock_obj.current.user_id = 'u' + + result = self.runner.invoke( + artifact, + ['list', '--versions', '--app_id', 'a'], + obj=mock_obj, + ) + assert result.exit_code != 0 + assert '--artifact_id' in result.output @patch('clarifai.cli.artifact.validate_context') @patch('clarifai.client.artifact_version.ArtifactVersion') From 7ac62d904083ab0c3e296399e1f31fa9eca415fe Mon Sep 17 00:00:00 2001 From: sharang chopra Date: Mon, 25 May 2026 07:55:18 +0530 Subject: [PATCH 2/4] Allow --artifact_id without --versions for single-row lookup Mirrors kubectl-style behavior: 'clarifai af list --app_id a --artifact_id x' fetches a single artifact via Artifact.get and displays it as a one-row table. --artifact_id still requires --app_id. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- clarifai/cli/artifact.py | 38 +++++++++++++++++++----- tests/cli/test_artifact_cli.py | 54 ++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 7 deletions(-) diff --git a/clarifai/cli/artifact.py b/clarifai/cli/artifact.py index f53663e3..086db810 100644 --- a/clarifai/cli/artifact.py +++ b/clarifai/cli/artifact.py @@ -291,6 +291,10 @@ def list(ctx, path, user_id, app_id, artifact_id, versions, page_no, per_page): # List versions of a specific artifact clarifai af list --versions --app_id my-app --artifact_id my-artifact + \b + # Look up a single artifact by id (kubectl-style) + clarifai af list --app_id my-app --artifact_id my-artifact + \b # Another user clarifai af list --user_id other-user --app_id their-app @@ -373,19 +377,39 @@ def list(ctx, path, user_id, app_id, artifact_id, versions, page_no, per_page): return # Listing artifacts (not versions). - if artifact_id: - click.echo( - "--artifact_id is only valid with --versions when listing.", - err=True, - ) - raise click.Abort() - list_kwargs = {} if page_no is not None: list_kwargs['page'] = page_no if per_page is not None: list_kwargs['per_page'] = per_page + if artifact_id: + # Single-artifact lookup mode (kubectl-style: pass the ID, get one row). + if not app_id: + click.echo( + "--artifact_id requires --app_id.", + err=True, + ) + raise click.Abort() + + artifact_client = Artifact( + user_id=user_id, + app_id=app_id, + pat=ctx.obj.current.pat, + base=ctx.obj.current.api_base, + ) + single = artifact_client.get(artifact_id=artifact_id) + display_co_resources( + [single], + custom_columns={ + 'ARTIFACT': lambda a: a.id, + 'LATEST_VERSION': lambda a: _resolve_latest_version_id(a), + 'VISIBILITY': lambda a: _resolve_visibility(resources_pb2, a), + 'CREATED_AT': lambda a: str(a.created_at.ToDatetime()) if a.created_at else '', + }, + ) + return + if app_id: # Single-app mode. artifact_client = Artifact( diff --git a/tests/cli/test_artifact_cli.py b/tests/cli/test_artifact_cli.py index 7ad2c750..eb65977c 100644 --- a/tests/cli/test_artifact_cli.py +++ b/tests/cli/test_artifact_cli.py @@ -300,6 +300,60 @@ def test_list_versions_requires_artifact_id(self, mock_validate): assert result.exit_code != 0 assert '--artifact_id' in result.output + @patch('clarifai.cli.artifact.validate_context') + @patch('clarifai.client.artifact.Artifact') + def test_list_with_artifact_id_returns_single_row(self, mock_artifact_class, mock_validate): + """--artifact_id without --versions does a single-artifact lookup.""" + mock_validate.return_value = None + + mock_artifact_instance = Mock() + single = Mock() + single.id = 'my-artifact' + single.artifact_version = Mock() + single.artifact_version.id = 'v1' + single.artifact_version.visibility.gettable = 10 + single.created_at = None + mock_artifact_instance.get.return_value = single + mock_artifact_class.return_value = mock_artifact_instance + + mock_obj = create_mock_context() + + result = self.runner.invoke( + artifact, + [ + 'list', + '--user_id', + 'u', + '--app_id', + 'a', + '--artifact_id', + 'my-artifact', + ], + obj=mock_obj, + ) + + if result.exit_code != 0: + print(f"Command failed with output: {result.output}") + + assert result.exit_code == 0 + mock_artifact_instance.get.assert_called_once_with(artifact_id='my-artifact') + mock_artifact_instance.list.assert_not_called() + + @patch('clarifai.cli.artifact.validate_context') + def test_list_artifact_id_requires_app_id(self, mock_validate): + """--artifact_id without --app_id should error.""" + mock_validate.return_value = None + mock_obj = create_mock_context() + mock_obj.current.user_id = 'u' + + result = self.runner.invoke( + artifact, + ['list', '--artifact_id', 'x'], + obj=mock_obj, + ) + assert result.exit_code != 0 + assert '--app_id' in result.output + @patch('clarifai.cli.artifact.validate_context') @patch('clarifai.client.artifact_version.ArtifactVersion') def test_list_versions_command(self, mock_artifact_version_class, mock_validate): From e1cf7b9af295b56fa1f8ae5e8b775833eb3c2945 Mon Sep 17 00:00:00 2001 From: sharang chopra Date: Mon, 25 May 2026 09:23:32 +0530 Subject: [PATCH 3/4] fixes --- clarifai/cli/artifact.py | 52 ++++++++++++++++++++++++----- clarifai/client/artifact.py | 46 ++++++++++++++++--------- clarifai/client/artifact_version.py | 46 ++++++++++++++++--------- tests/cli/test_artifact_cli.py | 50 +++++++++++++++++++++++++++ tests/test_artifact.py | 39 ++++++++++++++++++++++ tests/test_artifact_version.py | 41 +++++++++++++++++++++++ 6 files changed, 236 insertions(+), 38 deletions(-) diff --git a/clarifai/cli/artifact.py b/clarifai/cli/artifact.py index 086db810..b3f865d7 100644 --- a/clarifai/cli/artifact.py +++ b/clarifai/cli/artifact.py @@ -268,12 +268,36 @@ def artifact(): @click.option( '--app_id', required=False, - help="App ID. If omitted, lists artifacts across all of the user's apps.", + help=( + "App ID. If omitted, lists artifacts across all of the user's apps. " + "Required with --artifact_id." + ), +) +@click.option( + '--artifact_id', + required=False, + help=( + 'Artifact ID. With --versions, lists versions for this artifact. ' + 'Without --versions, lists the artifact itself. Requires --app_id.' + ), +) +@click.option( + '--versions', + is_flag=True, + help='List versions for the specified artifact instead of artifacts.', +) +@click.option( + '--page_no', + type=int, + default=None, + help='Page number to list. Omit to list all results.', +) +@click.option( + '--per_page', + type=int, + default=None, + help='Number of items per page.', ) -@click.option('--artifact_id', required=False, help='Artifact ID (required with --versions).') -@click.option('--versions', is_flag=True, help='List artifact versions instead of artifacts.') -@click.option('--page_no', type=int, default=None, help='Page number to list.') -@click.option('--per_page', type=int, default=None, help='Number of items per page.') @click.pass_context def list(ctx, path, user_id, app_id, artifact_id, versions, page_no, per_page): """List artifacts or artifact versions. @@ -287,12 +311,16 @@ def list(ctx, path, user_id, app_id, artifact_id, versions, page_no, per_page): # Scope to a single app clarifai af list --app_id my-app + \b + # Paginate within a single app + clarifai af list --app_id my-app --page_no 2 --per_page 50 + \b # List versions of a specific artifact clarifai af list --versions --app_id my-app --artifact_id my-artifact \b - # Look up a single artifact by id (kubectl-style) + # Look up a single artifact by id clarifai af list --app_id my-app --artifact_id my-artifact \b @@ -350,7 +378,7 @@ def list(ctx, path, user_id, app_id, artifact_id, versions, page_no, per_page): ) list_kwargs = {} if page_no is not None: - list_kwargs['page_no'] = page_no + list_kwargs['page'] = page_no if per_page is not None: list_kwargs['per_page'] = per_page versions_list = builtin_list(artifact_version.list(**list_kwargs)) @@ -435,6 +463,14 @@ def list(ctx, path, user_id, app_id, artifact_id, versions, page_no, per_page): ) return + if page_no is not None or per_page is not None: + click.echo( + "--page_no and --per_page require --app_id (or --versions). " + "Global pagination across all apps is not supported.", + err=True, + ) + raise click.Abort() + # User-default mode: iterate apps and aggregate artifacts. from clarifai.client.user import User @@ -449,7 +485,7 @@ def list(ctx, path, user_id, app_id, artifact_id, versions, page_no, per_page): return logger.info( - f"Listing artifacts across {len(apps)} app(s) for user {user_id}. " + f"Listing all artifacts across {len(apps)} app(s) for user {user_id}. " "Use --app_id to scope to a single app." ) diff --git a/clarifai/client/artifact.py b/clarifai/client/artifact.py index 01d600e6..3c3a60d0 100644 --- a/clarifai/client/artifact.py +++ b/clarifai/client/artifact.py @@ -1,4 +1,4 @@ -from typing import Generator +from typing import Generator, Optional from clarifai_grpc.grpc.api import resources_pb2, service_pb2 from clarifai_grpc.grpc.api.status import status_code_pb2 @@ -188,8 +188,8 @@ def list( self, user_id: str = "", app_id: str = "", - page: int = 1, - per_page: int = DEFAULT_ARTIFACTS_PAGE_SIZE, + page: Optional[int] = None, + per_page: Optional[int] = None, **kwargs, ) -> Generator[resources_pb2.Artifact, None, None]: """List artifacts in an app. @@ -197,8 +197,10 @@ def list( Args: user_id: The user ID. Defaults to the user ID from initialization. app_id: The app ID. Defaults to the app ID from initialization. - page: The page number for pagination. Defaults to 1. - per_page: The number of results per page. Defaults to 20. + page: The page number for pagination. If None and per_page is also None, + all pages are listed. + per_page: The number of results per page. If page is specified and per_page + is omitted, the default page size is used. **kwargs: Additional keyword arguments to be passed to the BaseClient. Yields: @@ -215,16 +217,30 @@ def list( if not app_id: raise UserError("app_id is required") - request = service_pb2.ListArtifactsRequest( - user_app_id=self.auth_helper.get_user_app_id_proto(user_id=user_id, app_id=app_id), - page=page, - per_page=per_page, - ) + request_page = 1 if page is None else page + request_per_page = per_page or DEFAULT_ARTIFACTS_PAGE_SIZE - response = self._grpc_request(self.STUB.ListArtifacts, request) + while True: + request = service_pb2.ListArtifactsRequest( + user_app_id=self.auth_helper.get_user_app_id_proto(user_id=user_id, app_id=app_id), + page=request_page, + per_page=request_per_page, + ) - if response.status.code != status_code_pb2.SUCCESS: - raise Exception(f"Failed to list artifacts: {response.status.description}") + response = self._grpc_request(self.STUB.ListArtifacts, request) + + if response.status.code != status_code_pb2.SUCCESS: + raise Exception(f"Failed to list artifacts: {response.status.description}") + + if not response.artifacts: + break + + for artifact_pb in response.artifacts: + yield artifact_pb + + if page is not None or per_page is not None: + break + if len(response.artifacts) < request_per_page: + break - for artifact_pb in response.artifacts: - yield artifact_pb + request_page += 1 diff --git a/clarifai/client/artifact_version.py b/clarifai/client/artifact_version.py index ba376fc7..8e72ac24 100644 --- a/clarifai/client/artifact_version.py +++ b/clarifai/client/artifact_version.py @@ -684,8 +684,8 @@ def list( artifact_id: str = "", user_id: str = "", app_id: str = "", - page: int = 1, - per_page: int = DEFAULT_ARTIFACTS_PAGE_SIZE, + page: Optional[int] = None, + per_page: Optional[int] = None, **kwargs, ) -> Generator[resources_pb2.ArtifactVersion, None, None]: """List artifact versions for this artifact. @@ -694,8 +694,10 @@ def list( artifact_id: The artifact ID. Defaults to the artifact ID from initialization. user_id: The user ID. Defaults to the user ID from initialization. app_id: The app ID. Defaults to the app ID from initialization. - page: The page number for pagination. Defaults to 1. - per_page: The number of results per page. Defaults to 20. + page: The page number for pagination. If None and per_page is also None, + all pages are listed. + per_page: The number of results per page. If page is specified and per_page + is omitted, the default page size is used. **kwargs: Additional keyword arguments to be passed to the BaseClient. Yields: @@ -716,17 +718,31 @@ def list( if not app_id: raise UserError("app_id is required") - request = service_pb2.ListArtifactVersionsRequest( - user_app_id=self.auth_helper.get_user_app_id_proto(user_id=user_id, app_id=app_id), - artifact_id=artifact_id, - page=page, - per_page=per_page, - ) + request_page = 1 if page is None else page + request_per_page = per_page or DEFAULT_ARTIFACTS_PAGE_SIZE - response = self._grpc_request(self.STUB.ListArtifactVersions, request) + while True: + request = service_pb2.ListArtifactVersionsRequest( + user_app_id=self.auth_helper.get_user_app_id_proto(user_id=user_id, app_id=app_id), + artifact_id=artifact_id, + page=request_page, + per_page=request_per_page, + ) - if response.status.code != status_code_pb2.SUCCESS: - raise Exception(f"Failed to list artifact versions: {response.status.description}") + response = self._grpc_request(self.STUB.ListArtifactVersions, request) + + if response.status.code != status_code_pb2.SUCCESS: + raise Exception(f"Failed to list artifact versions: {response.status.description}") + + if not response.artifact_versions: + break + + for version_pb in response.artifact_versions: + yield version_pb + + if page is not None or per_page is not None: + break + if len(response.artifact_versions) < request_per_page: + break - for version_pb in response.artifact_versions: - yield version_pb + request_page += 1 diff --git a/tests/cli/test_artifact_cli.py b/tests/cli/test_artifact_cli.py index eb65977c..7bb3ec13 100644 --- a/tests/cli/test_artifact_cli.py +++ b/tests/cli/test_artifact_cli.py @@ -284,6 +284,41 @@ def test_list_versions_with_flags(self, mock_artifact_version_class, mock_valida assert kwargs['app_id'] == 'test_app' assert kwargs['artifact_id'] == 'test_artifact' assert result.exit_code == 0 + mock_version_instance.list.assert_called_once_with() + + @patch('clarifai.cli.artifact.validate_context') + @patch('clarifai.client.artifact_version.ArtifactVersion') + def test_list_versions_pagination_with_flags(self, mock_artifact_version_class, mock_validate): + """Version pagination should forward page/per_page correctly.""" + mock_validate.return_value = None + + mock_version_instance = Mock() + mock_version_instance.list.return_value = [] + mock_artifact_version_class.return_value = mock_version_instance + + mock_obj = create_mock_context() + + result = self.runner.invoke( + artifact, + [ + 'list', + '--versions', + '--user_id', + 'test_user', + '--app_id', + 'test_app', + '--artifact_id', + 'test_artifact', + '--page_no', + '2', + '--per_page', + '5', + ], + obj=mock_obj, + ) + + assert result.exit_code == 0 + mock_version_instance.list.assert_called_once_with(page=2, per_page=5) @patch('clarifai.cli.artifact.validate_context') def test_list_versions_requires_artifact_id(self, mock_validate): @@ -354,6 +389,21 @@ def test_list_artifact_id_requires_app_id(self, mock_validate): assert result.exit_code != 0 assert '--app_id' in result.output + @patch('clarifai.cli.artifact.validate_context') + def test_list_cross_app_pagination_requires_app_id(self, mock_validate): + """Cross-app listing should reject pagination flags.""" + mock_validate.return_value = None + mock_obj = create_mock_context() + mock_obj.current.user_id = 'u' + + result = self.runner.invoke( + artifact, + ['list', '--page_no', '2'], + obj=mock_obj, + ) + assert result.exit_code != 0 + assert 'Global pagination across all apps is not supported' in result.output + @patch('clarifai.cli.artifact.validate_context') @patch('clarifai.client.artifact_version.ArtifactVersion') def test_list_versions_command(self, mock_artifact_version_class, mock_validate): diff --git a/tests/test_artifact.py b/tests/test_artifact.py index 40215902..9565fd4b 100644 --- a/tests/test_artifact.py +++ b/tests/test_artifact.py @@ -6,6 +6,7 @@ from clarifai_grpc.grpc.api import resources_pb2 from clarifai.client.artifact import Artifact +from clarifai.constants.artifact import DEFAULT_ARTIFACTS_PAGE_SIZE from clarifai.errors import UserError @@ -213,6 +214,44 @@ def test_list_success(self): call_args = mock_grpc_request.call_args assert call_args[0][0] == mock_stub.ListArtifacts + def test_list_all_pages_by_default(self): + """Test artifact listing fetches all pages when pagination is omitted.""" + first_page = Mock() + first_page.status.code = 10000 # SUCCESS + first_page.artifacts = [ + Mock(id=f"artifact-{i}") for i in range(DEFAULT_ARTIFACTS_PAGE_SIZE) + ] + + second_page = Mock() + second_page.status.code = 10000 # SUCCESS + last_artifact = Mock() + last_artifact.id = "artifact-last" + second_page.artifacts = [last_artifact] + + with ( + patch('clarifai.client.base.BaseClient.__init__', return_value=None), + patch.object(Artifact, '_grpc_request') as mock_grpc_request, + ): + mock_grpc_request.side_effect = [first_page, second_page] + + artifact = Artifact(user_id="test_user", app_id="test_app") + mock_auth_helper = Mock() + mock_auth_helper.get_user_app_id_proto.return_value = resources_pb2.UserAppIDSet( + user_id="test_user", app_id="test_app" + ) + artifact.auth_helper = mock_auth_helper + mock_stub = Mock() + mock_stub.ListArtifacts = Mock() + artifact.STUB = mock_stub + + artifacts = list(artifact.list()) + + assert len(artifacts) == DEFAULT_ARTIFACTS_PAGE_SIZE + 1 + assert artifacts[-1].id == "artifact-last" + assert mock_grpc_request.call_count == 2 + assert mock_grpc_request.call_args_list[0][0][1].page == 1 + assert mock_grpc_request.call_args_list[1][0][1].page == 2 + def test_create_missing_artifact_id(self): """Test artifact creation with missing artifact_id (now required).""" artifact = self._create_mock_artifact() diff --git a/tests/test_artifact_version.py b/tests/test_artifact_version.py index 7d1932d7..462de7a6 100644 --- a/tests/test_artifact_version.py +++ b/tests/test_artifact_version.py @@ -8,6 +8,7 @@ from google.protobuf import timestamp_pb2 from clarifai.client.artifact_version import ArtifactVersion +from clarifai.constants.artifact import DEFAULT_ARTIFACTS_PAGE_SIZE from clarifai.errors import UserError from clarifai.utils.misc import format_bytes from tests.test_artifact_utils import create_mock_artifact_version @@ -432,6 +433,46 @@ def test_list_success(self): assert results[1].id == "version2" mock_grpc_request.assert_called_once() + def test_list_all_pages_by_default(self): + """Test version listing fetches all pages when pagination is omitted.""" + first_page = Mock() + first_page.status.code = 10000 # SUCCESS + first_page.artifact_versions = [ + Mock(id=f"version-{i}") for i in range(DEFAULT_ARTIFACTS_PAGE_SIZE) + ] + + second_page = Mock() + second_page.status.code = 10000 # SUCCESS + last_version = Mock() + last_version.id = "version-last" + second_page.artifact_versions = [last_version] + + with ( + patch('clarifai.client.base.BaseClient.__init__', return_value=None), + patch.object(ArtifactVersion, '_grpc_request') as mock_grpc_request, + ): + mock_grpc_request.side_effect = [first_page, second_page] + + version = ArtifactVersion( + artifact_id="test_artifact", user_id="test_user", app_id="test_app" + ) + mock_auth_helper = Mock() + mock_auth_helper.get_user_app_id_proto.return_value = resources_pb2.UserAppIDSet( + user_id="test_user", app_id="test_app" + ) + version.auth_helper = mock_auth_helper + mock_stub = Mock() + mock_stub.ListArtifactVersions = Mock() + version.STUB = mock_stub + + results = list(version.list()) + + assert len(results) == DEFAULT_ARTIFACTS_PAGE_SIZE + 1 + assert results[-1].id == "version-last" + assert mock_grpc_request.call_count == 2 + assert mock_grpc_request.call_args_list[0][0][1].page == 1 + assert mock_grpc_request.call_args_list[1][0][1].page == 2 + def test_list_missing_params(self): """Test list with missing required parameters.""" with pytest.raises(UserError, match="artifact_id is required"): From 1b0f550620dca138ed425b4d41d8f6bcc88d9474 Mon Sep 17 00:00:00 2001 From: sharang chopra Date: Mon, 25 May 2026 09:52:02 +0530 Subject: [PATCH 4/4] Review comments --- clarifai/cli/artifact.py | 14 ++++++++++---- clarifai/client/artifact.py | 4 ++++ clarifai/client/artifact_version.py | 4 ++++ 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/clarifai/cli/artifact.py b/clarifai/cli/artifact.py index b3f865d7..aa9dcc27 100644 --- a/clarifai/cli/artifact.py +++ b/clarifai/cli/artifact.py @@ -288,15 +288,19 @@ def artifact(): ) @click.option( '--page_no', - type=int, + type=click.IntRange(min=1), default=None, - help='Page number to list. Omit to list all results.', + help=( + 'Page number to list (must be >= 1). ' + 'Omit both --page_no and --per_page to list all results. ' + 'Supported with --app_id or --versions.' + ), ) @click.option( '--per_page', - type=int, + type=click.IntRange(min=1), default=None, - help='Number of items per page.', + help=('Number of items per page (must be >= 1). Supported with --app_id or --versions.'), ) @click.pass_context def list(ctx, path, user_id, app_id, artifact_id, versions, page_no, per_page): @@ -522,6 +526,8 @@ def list(ctx, path, user_id, app_id, artifact_id, versions, page_no, per_page): }, ) + except click.Abort: + raise except UserError as e: click.echo(str(e), err=True) raise click.Abort() diff --git a/clarifai/client/artifact.py b/clarifai/client/artifact.py index 3c3a60d0..307f9ff4 100644 --- a/clarifai/client/artifact.py +++ b/clarifai/client/artifact.py @@ -216,6 +216,10 @@ def list( raise UserError("user_id is required") if not app_id: raise UserError("app_id is required") + if page is not None and page < 1: + raise UserError("page must be >= 1") + if per_page is not None and per_page < 1: + raise UserError("per_page must be >= 1") request_page = 1 if page is None else page request_per_page = per_page or DEFAULT_ARTIFACTS_PAGE_SIZE diff --git a/clarifai/client/artifact_version.py b/clarifai/client/artifact_version.py index 8e72ac24..cd75d811 100644 --- a/clarifai/client/artifact_version.py +++ b/clarifai/client/artifact_version.py @@ -717,6 +717,10 @@ def list( raise UserError("user_id is required") if not app_id: raise UserError("app_id is required") + if page is not None and page < 1: + raise UserError("page must be >= 1") + if per_page is not None and per_page < 1: + raise UserError("per_page must be >= 1") request_page = 1 if page is None else page request_per_page = per_page or DEFAULT_ARTIFACTS_PAGE_SIZE