From 367c08f01e7f6c5b7e1365784a093bf3da97ea77 Mon Sep 17 00:00:00 2001 From: Ihar Hrachyshka Date: Tue, 25 Mar 2025 20:12:36 -0400 Subject: [PATCH] feat(api): don't return a payload on file delete (#1640) # What does this PR do? This is to stay consistent with other APIs. This change registers files in API, even though there are still no providers. Removing tests that require a provider existing for a merged API to enable it in API layer. Signed-off-by: Ihar Hrachyshka [//]: # (If resolving an issue, uncomment and update the line below) [//]: # (Closes #[issue-number]) ## Test Plan [Describe the tests you ran to verify your changes with result summaries. *Provide clear instructions so the plan can be easily re-executed.*] [//]: # (## Documentation) Signed-off-by: Ihar Hrachyshka --- docs/_static/llama-stack-spec.html | 89 ++++++++++----------- docs/_static/llama-stack-spec.yaml | 70 ++++++++-------- docs/openapi_generator/generate.py | 5 +- docs/openapi_generator/pyopenapi/utility.py | 75 +++++++++++------ llama_stack/apis/datatypes.py | 1 + llama_stack/apis/files/files.py | 2 +- llama_stack/distribution/resolver.py | 2 + llama_stack/providers/registry/files.py | 11 +++ tests/unit/providers/test_configs.py | 7 -- 9 files changed, 143 insertions(+), 119 deletions(-) create mode 100644 llama_stack/providers/registry/files.py diff --git a/docs/_static/llama-stack-spec.html b/docs/_static/llama-stack-spec.html index 7b1b6a0b6..4990d845e 100644 --- a/docs/_static/llama-stack-spec.html +++ b/docs/_static/llama-stack-spec.html @@ -818,14 +818,7 @@ "delete": { "responses": { "200": { - "description": "OK", - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/FileResponse" - } - } - } + "description": "OK" }, "400": { "$ref": "#/components/responses/BadRequest400" @@ -6140,46 +6133,6 @@ "title": "FileUploadResponse", "description": "Response after initiating a file upload session." }, - "FileResponse": { - "type": "object", - "properties": { - "bucket": { - "type": "string", - "description": "Bucket under which the file is stored (valid chars: a-zA-Z0-9_-)" - }, - "key": { - "type": "string", - "description": "Key under which the file is stored (valid chars: a-zA-Z0-9_-/.)" - }, - "mime_type": { - "type": "string", - "description": "MIME type of the file" - }, - "url": { - "type": "string", - "description": "Upload URL for the file contents" - }, - "bytes": { - "type": "integer", - "description": "Size of the file in bytes" - }, - "created_at": { - "type": "integer", - "description": "Timestamp of when the file was created" - } - }, - "additionalProperties": false, - "required": [ - "bucket", - "key", - "mime_type", - "url", - "bytes", - "created_at" - ], - "title": "FileResponse", - "description": "Response representing a file entry." - }, "EmbeddingsRequest": { "type": "object", "properties": { @@ -6933,6 +6886,46 @@ "title": "URIDataSource", "description": "A dataset that can be obtained from a URI." }, + "FileResponse": { + "type": "object", + "properties": { + "bucket": { + "type": "string", + "description": "Bucket under which the file is stored (valid chars: a-zA-Z0-9_-)" + }, + "key": { + "type": "string", + "description": "Key under which the file is stored (valid chars: a-zA-Z0-9_-/.)" + }, + "mime_type": { + "type": "string", + "description": "MIME type of the file" + }, + "url": { + "type": "string", + "description": "Upload URL for the file contents" + }, + "bytes": { + "type": "integer", + "description": "Size of the file in bytes" + }, + "created_at": { + "type": "integer", + "description": "Timestamp of when the file was created" + } + }, + "additionalProperties": false, + "required": [ + "bucket", + "key", + "mime_type", + "url", + "bytes", + "created_at" + ], + "title": "FileResponse", + "description": "Response representing a file entry." + }, "Model": { "type": "object", "properties": { diff --git a/docs/_static/llama-stack-spec.yaml b/docs/_static/llama-stack-spec.yaml index a815931de..ba3868560 100644 --- a/docs/_static/llama-stack-spec.yaml +++ b/docs/_static/llama-stack-spec.yaml @@ -557,10 +557,6 @@ paths: responses: '200': description: OK - content: - application/json: - schema: - $ref: '#/components/schemas/FileResponse' '400': $ref: '#/components/responses/BadRequest400' '429': @@ -4286,39 +4282,6 @@ components: title: FileUploadResponse description: >- Response after initiating a file upload session. - FileResponse: - type: object - properties: - bucket: - type: string - description: >- - Bucket under which the file is stored (valid chars: a-zA-Z0-9_-) - key: - type: string - description: >- - Key under which the file is stored (valid chars: a-zA-Z0-9_-/.) - mime_type: - type: string - description: MIME type of the file - url: - type: string - description: Upload URL for the file contents - bytes: - type: integer - description: Size of the file in bytes - created_at: - type: integer - description: Timestamp of when the file was created - additionalProperties: false - required: - - bucket - - key - - mime_type - - url - - bytes - - created_at - title: FileResponse - description: Response representing a file entry. EmbeddingsRequest: type: object properties: @@ -4830,6 +4793,39 @@ components: title: URIDataSource description: >- A dataset that can be obtained from a URI. + FileResponse: + type: object + properties: + bucket: + type: string + description: >- + Bucket under which the file is stored (valid chars: a-zA-Z0-9_-) + key: + type: string + description: >- + Key under which the file is stored (valid chars: a-zA-Z0-9_-/.) + mime_type: + type: string + description: MIME type of the file + url: + type: string + description: Upload URL for the file contents + bytes: + type: integer + description: Size of the file in bytes + created_at: + type: integer + description: Timestamp of when the file was created + additionalProperties: false + required: + - bucket + - key + - mime_type + - url + - bytes + - created_at + title: FileResponse + description: Response representing a file entry. Model: type: object properties: diff --git a/docs/openapi_generator/generate.py b/docs/openapi_generator/generate.py index 879ac95e2..a3c27cf4e 100644 --- a/docs/openapi_generator/generate.py +++ b/docs/openapi_generator/generate.py @@ -21,7 +21,7 @@ from llama_stack.distribution.stack import LlamaStack # noqa: E402 from .pyopenapi.options import Options # noqa: E402 from .pyopenapi.specification import Info, Server # noqa: E402 -from .pyopenapi.utility import Specification, validate_api_method_return_types # noqa: E402 +from .pyopenapi.utility import Specification, validate_api # noqa: E402 def str_presenter(dumper, data): @@ -40,8 +40,7 @@ def main(output_dir: str): raise ValueError(f"Directory {output_dir} does not exist") # Validate API protocols before generating spec - print("Validating API method return types...") - return_type_errors = validate_api_method_return_types() + return_type_errors = validate_api() if return_type_errors: print("\nAPI Method Return Type Validation Errors:\n") for error in return_type_errors: diff --git a/docs/openapi_generator/pyopenapi/utility.py b/docs/openapi_generator/pyopenapi/utility.py index f60a33bb7..cb155fed7 100644 --- a/docs/openapi_generator/pyopenapi/utility.py +++ b/docs/openapi_generator/pyopenapi/utility.py @@ -7,10 +7,9 @@ import json import typing import inspect -import os from pathlib import Path from typing import TextIO -from typing import Any, Dict, List, Optional, Protocol, Type, Union, get_type_hints, get_origin, get_args +from typing import Any, List, Optional, Union, get_type_hints, get_origin, get_args from llama_stack.strong_typing.schema import object_to_json, StrictJsonType from llama_stack.distribution.resolver import api_protocol_map @@ -125,29 +124,59 @@ def is_optional_type(type_: Any) -> bool: return origin is Optional or (origin is Union and type(None) in args) -def validate_api_method_return_types() -> List[str]: - """Validate that all API methods have proper return types.""" +def _validate_api_method_return_type(method) -> str | None: + hints = get_type_hints(method) + + if 'return' not in hints: + return "has no return type annotation" + + return_type = hints['return'] + if is_optional_type(return_type): + return "returns Optional type" + + +def _validate_api_delete_method_returns_none(method) -> str | None: + hints = get_type_hints(method) + + if 'return' not in hints: + return "has no return type annotation" + + return_type = hints['return'] + if return_type is not None and return_type is not type(None): + return "does not return None" + + +_VALIDATORS = { + "GET": [ + _validate_api_method_return_type, + ], + "DELETE": [ + _validate_api_delete_method_returns_none, + ], +} + + +def _get_methods_by_type(protocol, method_type: str): + members = inspect.getmembers(protocol, predicate=inspect.isfunction) + return { + method_name: method + for method_name, method in members + if (webmethod := getattr(method, '__webmethod__', None)) + if webmethod and webmethod.method == method_type + } + + +def validate_api() -> List[str]: + """Validate the API protocols.""" errors = [] protocols = api_protocol_map() - for protocol_name, protocol in protocols.items(): - methods = inspect.getmembers(protocol, predicate=inspect.isfunction) - - for method_name, method in methods: - if not hasattr(method, '__webmethod__'): - continue - - # Only check GET methods - if method.__webmethod__.method != "GET": - continue - - hints = get_type_hints(method) - - if 'return' not in hints: - errors.append(f"Method {protocol_name}.{method_name} has no return type annotation") - else: - return_type = hints['return'] - if is_optional_type(return_type): - errors.append(f"Method {protocol_name}.{method_name} returns Optional type") + for target, validators in _VALIDATORS.items(): + for protocol_name, protocol in protocols.items(): + for validator in validators: + for method_name, method in _get_methods_by_type(protocol, target).items(): + err = validator(method) + if err: + errors.append(f"Method {protocol_name}.{method_name} {err}") return errors diff --git a/llama_stack/apis/datatypes.py b/llama_stack/apis/datatypes.py index f644e5137..25f3ab1ab 100644 --- a/llama_stack/apis/datatypes.py +++ b/llama_stack/apis/datatypes.py @@ -34,6 +34,7 @@ class Api(Enum): scoring_functions = "scoring_functions" benchmarks = "benchmarks" tool_groups = "tool_groups" + files = "files" # built-in API inspect = "inspect" diff --git a/llama_stack/apis/files/files.py b/llama_stack/apis/files/files.py index 65c1ead6a..ef8b65829 100644 --- a/llama_stack/apis/files/files.py +++ b/llama_stack/apis/files/files.py @@ -164,7 +164,7 @@ class Files(Protocol): self, bucket: str, key: str, - ) -> FileResponse: + ) -> None: """ Delete a file identified by a bucket and key. diff --git a/llama_stack/distribution/resolver.py b/llama_stack/distribution/resolver.py index e9e406699..25fe3f184 100644 --- a/llama_stack/distribution/resolver.py +++ b/llama_stack/distribution/resolver.py @@ -12,6 +12,7 @@ from llama_stack.apis.benchmarks import Benchmarks from llama_stack.apis.datasetio import DatasetIO from llama_stack.apis.datasets import Datasets from llama_stack.apis.eval import Eval +from llama_stack.apis.files import Files from llama_stack.apis.inference import Inference from llama_stack.apis.inspect import Inspect from llama_stack.apis.models import Models @@ -79,6 +80,7 @@ def api_protocol_map() -> Dict[Api, Any]: Api.post_training: PostTraining, Api.tool_groups: ToolGroups, Api.tool_runtime: ToolRuntime, + Api.files: Files, } diff --git a/llama_stack/providers/registry/files.py b/llama_stack/providers/registry/files.py new file mode 100644 index 000000000..fb23436bb --- /dev/null +++ b/llama_stack/providers/registry/files.py @@ -0,0 +1,11 @@ +# Copyright (c) Meta Platforms, Inc. and affiliates. +# All rights reserved. +# +# This source code is licensed under the terms described in the LICENSE file in +# the root directory of this source tree. + +from llama_stack.providers.datatypes import ProviderSpec + + +def available_providers() -> list[ProviderSpec]: + return [] diff --git a/tests/unit/providers/test_configs.py b/tests/unit/providers/test_configs.py index 246470372..99081c8b0 100644 --- a/tests/unit/providers/test_configs.py +++ b/tests/unit/providers/test_configs.py @@ -14,17 +14,10 @@ from llama_stack.distribution.utils.dynamic import instantiate_class_type class TestProviderConfigurations: """Test suite for testing provider configurations across all API types.""" - def test_all_api_providers_exist(self): - provider_registry = get_provider_registry() - for api in providable_apis(): - providers = provider_registry.get(api, {}) - assert providers, f"No providers found for API type: {api}" - @pytest.mark.parametrize("api", providable_apis()) def test_api_providers(self, api): provider_registry = get_provider_registry() providers = provider_registry.get(api, {}) - assert providers, f"No providers found for API type: {api}" failures = [] for provider_type, provider_spec in providers.items():