python(feat): sift_client consistency (#312)

Author: alexluck-sift

.githooks/pre-push

@@ -1,61 +1,24 @@
 #!/usr/bin/env bash
 
-# ensure generated python stubs are up-to-date, from sync clients and sift_stream_bindings
-
 # Store the root directory of the repository
 REPO_ROOT="$(git rev-parse --show-toplevel)"
-PYTHON_DIR="$REPO_ROOT/python"
-BINDINGS_DIR="$REPO_ROOT/rust/crates/sift_stream_bindings"
-STUBS_DIR="$PYTHON_DIR/lib/sift_client/resources/sync_stubs"
-
-# Function to check if generated stub files have changed
-check_stub_changes() {
-    local target_path="$1"
-    local changed_files=$(git status --porcelain "$target_path" | grep -E '\.pyi$' || true)
-
-    if [ -n "$changed_files" ]; then
-        echo "ERROR: Generated python stubs are not up-to-date. Please commit the changed files:"
-        echo "$changed_files"
-        exit 1
-    fi
-}
-
-# Function to generate Python stubs
-generate_python_stubs() {
-    echo "Generating Python stubs..."
-    cd "$PYTHON_DIR"
-
-    if [[ ! -d "$PYTHON_DIR/venv" ]]; then
-        echo "Running bootstrap script..."
-        bash ./scripts/dev bootstrap
-    fi
-
-    bash ./scripts/dev gen-stubs
-    check_stub_changes "$STUBS_DIR"
-}
+GITHOOKS_DIR="$REPO_ROOT/.githooks"
 
-# Function to generate bindings stubs
-generate_bindings_stubs() {
-    echo "Generating bindings stubs..."
-    cd "$BINDINGS_DIR"
-    cargo run --bin stub_gen
-
-    # The stub file is generated in the bindings directory
-    local stub_file="$BINDINGS_DIR/sift_stream_bindings.pyi"
-    check_stub_changes "$stub_file"
-}
-
-# Check for changes in relevant files
+# Check for changes in Python files
 python_changed_files=($(git diff --name-only --diff-filter=ACM | grep '^python/lib/sift_client/' || true))
-bindings_changed_files=($(git diff --name-only --diff-filter=ACM | grep '^rust/crates/sift_stream_bindings/src/' || true))
 
-# Generate stubs if needed
 if [[ -n "$python_changed_files" ]]; then
-    generate_python_stubs
+    echo "Python files changed, running Python stub checks..."
+    bash "$GITHOOKS_DIR/pre-push-python/stubs.sh"
 fi
 
+# Check for changes in Rust bindings files
+bindings_changed_files=($(git diff --name-only --diff-filter=ACM | grep '^rust/crates/sift_stream_bindings/src/' || true))
+
 if [[ -n "$bindings_changed_files" ]]; then
-    generate_bindings_stubs
+    echo "Rust bindings files changed, running Rust stub checks..."
+    bash "$GITHOOKS_DIR/pre-push-rust/stubs.sh"
 fi
 
-echo "All stubs are up-to-date."
+echo "Pre-push checks completed successfully."
+

.githooks/pre-push-python/stubs.sh

@@ -0,0 +1,36 @@
+# ensure generated python stubs are up-to-date, from sync clients
+
+# Store the root directory of the repository
+REPO_ROOT="$(git rev-parse --show-toplevel)"
+PYTHON_DIR="$REPO_ROOT/python"
+STUBS_DIR="$PYTHON_DIR/lib/sift_client/resources/sync_stubs"
+
+# Function to check if generated stub files have changed
+check_stub_changes() {
+    local target_path="$1"
+    local changed_files=$(git status --porcelain "$target_path" | grep -E '\.pyi$' || true)
+
+    if [ -n "$changed_files" ]; then
+        echo "ERROR: Generated python stubs are not up-to-date. Please commit the changed files:"
+        echo "$changed_files"
+        exit 1
+    fi
+}
+
+# Function to generate Python stubs
+generate_python_stubs() {
+    echo "Generating Python stubs..."
+    cd "$PYTHON_DIR"
+
+    if [[ ! -d "$PYTHON_DIR/venv" ]]; then
+        echo "Running bootstrap script..."
+        bash ./scripts/dev bootstrap
+    fi
+
+    bash ./scripts/dev gen-stubs
+    check_stub_changes "$STUBS_DIR"
+}
+
+generate_python_stubs
+
+echo "All stubs are up-to-date."

.githooks/pre-push-rust/stubs.sh

@@ -0,0 +1,32 @@
+# ensure generated python stubs are up-to-date, from sift_stream_bindings
+
+# Store the root directory of the repository
+REPO_ROOT="$(git rev-parse --show-toplevel)"
+BINDINGS_DIR="$REPO_ROOT/rust/crates/sift_stream_bindings"
+
+# Function to check if generated stub files have changed
+check_stub_changes() {
+    local target_path="$1"
+    local changed_files=$(git status --porcelain "$target_path" | grep -E '\.pyi$' || true)
+
+    if [ -n "$changed_files" ]; then
+        echo "ERROR: Generated python stubs are not up-to-date. Please commit the changed files:"
+        echo "$changed_files"
+        exit 1
+    fi
+}
+
+# Function to generate bindings stubs
+generate_bindings_stubs() {
+    echo "Generating bindings stubs..."
+    cd "$BINDINGS_DIR"
+    cargo run --bin stub_gen
+
+    # The stub file is generated in the bindings directory
+    local stub_file="$BINDINGS_DIR/sift_stream_bindings.pyi"
+    check_stub_changes "$stub_file"
+}
+
+generate_bindings_stubs
+
+echo "All stubs are up-to-date."

python/lib/sift_client/.ruff.toml

@@ -49,6 +49,7 @@ ignore = ["W191", "D206", "D300", # https://docs.astral.sh/ruff/formatter/#confl
     "D105", # Missing docstring in magic method
     "D205", # 1 blank line required between summary line and description
     "D100", # Missing docstring in public module
+    "C408", # Allow dict()
 ]
 
 

python/lib/sift_client/_internal/CONTRIBUTING.md

@@ -46,12 +46,48 @@ All low-level clients should implement `LowLevelClientBase` from `sift_client/_i
 
 ### Sift Types
 
-New Sift types can be implemented in `sift_client/types`.
+New Sift types can be implemented in `sift_client/sift_types`.
 
 These types are used to define Pydantic models for all domain objects and to convert between protocol buffers and Python. Additional
-update models can be implemented for performing updates with field masks.
+update and create models can be implemented for performing updates with field masks.
 
-All Sift types should inherit from `BaseType` and model updates from `ModelUpdate` in `sift_client/types/_base.py`
+All Sift types should inherit from `BaseType` in `sift_client/sift_types/_base.py`
+
+#### Create/Update Pydantic Model Inheritance Pattern
+
+The Sift client uses a composition-based inheritance pattern for Pydantic models to avoid complex multiple inheritance issues:
+
+1. **Base Classes** (`sift_client/sift_types/_base.py`):
+   - `ModelCreateUpdateBase`: Base class containing shared functionality for proto conversion and field mapping
+   - `ModelCreate`: Inherits from `ModelCreateUpdateBase` with generic typing for creation operations
+   - `ModelUpdate`: Inherits from `ModelCreateUpdateBase` with additional field mask support for updates
+
+2. **Domain-Specific Base Classes**:
+   Create a base class that inherits from `ModelCreateUpdateBase` and contains:
+   - All shared field definitions
+   - Shared `_to_proto_helpers` configuration for complex proto mappings
+   - Common validation logic using `@model_validator`
+   It may not always make sense to implement a base class if there is little/no overlap in fields or protos.
+
+3. **Create and Update Models**:
+   - `{Domain}Create`: Inherits from both `{Domain}Base` and `ModelCreate[{CreateProto}]`
+     - Include create only fields and validators
+   - `{Domain}Update`: Inherits from both `{Domain}Base` and `ModelUpdate[{UpdateProto}]`
+     - Include update only fields and validators
+
+#### Proto Mapping Helpers
+
+Use `MappingHelper` for complex proto field mappings when the Pydantic model doesn't match the proto model exactly:
+- `proto_attr_path`: Dot-separated path to the proto field
+- `update_field`: Field name for update masks (optional)
+- `converter`: Function/class to convert the value (optional)
+
+#### Validation Guidelines
+
+- Use `@model_validator(mode="after")` for cross-field validation
+- Prefix validation method names with `_` (e.g., `_validate_time_fields`) since these don't need to be user visible
+- Keep validation logic in the base class when shared between create/update
+- Add specific validation in create/update classes as needed
 
 ### High-Level APIs
 
@@ -62,6 +98,62 @@ Static and class methods should be avoided since these cannot have associated sy
 
 All high-level APIs should inherit from `ResourceBase` from `sift_client/resources/_base.py`.
 
+#### Resource Method Patterns
+
+Resource classes should implement consistent patterns for common operations. Use the helper methods from `ResourceBase` to build standard filter arguments.
+
+**Important:** Arguments that represent another Sift Type should always accept both the object instance and its ID string. This provides flexibility for users who may have either form.
+
+
+**Note**: If the proto API does not support filters for a resource, the API should be updated to make the resource filterable in a consistent way with other resources.
+
+Examples:
+```python
+# Accept either Asset object or asset ID string
+async def update(self, asset: Asset | str, ...) -> Asset:
+```
+
+##### Standard Method Signatures
+
+**`get(resource_id: str) -> {Type}`**
+- Single required positional argument for the resource ID
+- Returns the specific resource instance
+
+**`list_(...) -> list[{Type}]`**
+- Use `list_` (with underscore) to avoid conflicts with Python's built-in `list`
+- Standard filter arguments in consistent order (as applicable:)
+  1. Name filters: `name`, `name_contains`, `name_regex`
+  2. Self IDs: Resource-specific ID filters (e.g., `run_ids`, `asset_ids`, `client_keys`)
+  3. Created/modified ranges: `created_after`, `created_before`, `modified_after`, `modified_before`
+  4. Created/modified users: `created_by`, `modified_by`
+  5. Metadata: `metadata`, `tags`
+  6. Resource-specific filters: Domain-specific filters (e.g., `assets`, `duration_less_than`, `start_time_after`)
+  7. Common filters: `description_contains`, `include_archived`, `filter_query`
+  8. Ordering and pagination: `order_by`, `limit`, `page_size`, `page_token`
+
+**`find(...) -> {Type} | None`**
+- Similar signature to `list_` but returns single result or None
+- Should use the same filter arguments as `list_`
+
+**`create(create: {Type}Create | dict, **kwargs) -> {Type}`**
+- Accept both Pydantic model and dict
+- Additional keyword arguments for operation-specific options
+
+**`update({resource}: str | {Type}, update: {Type}Update | dict, **kwargs) -> {Type}`**
+- First argument accepts either ID string or resource instance
+- Update model as second argument
+- Additional keyword arguments for operation-specific options
+
+##### Using ResourceBase Helper Methods
+
+The `ResourceBase` class provides helper methods to build consistent CEL filter expressions:
+
+- `_build_name_cel_filters()`: Handles `name`, `name_contains`, `name_regex`
+- `_build_time_cel_filters()`: Handles time-based filters and user filters
+- `_build_tags_metadata_cel_filters()`: Handles `tags` and `metadata` filters
+- `_build_common_cel_filters()`: Handles `description_contains`, `include_archived`, `filter_query`
+
+
 #### Sync API Generation
 
 To generate a sync API from an async API, add a `generate_sync_api` function call in `sift_client/resources/sync_stubs/__init__.py` and

python/lib/sift_client/_internal/gen_pyi.py

@@ -143,7 +143,9 @@ def generate_stubs_for_module(path_arg: str | pathlib.Path) -> dict[pathlib.Path
             raw_doc = inspect.getdoc(cls) or ""
             if raw_doc:
                 doc = (
-                    '    """\n' + "\n".join(f"    {l}" for l in raw_doc.splitlines()) + '\n    """'
+                    '    """\n'
+                    + "\n".join(f"    {l.strip()}" for l in raw_doc.splitlines())
+                    + '\n    """'
                 )
             else:
                 doc = "    ..."

python/lib/sift_client/_internal/low_level_wrappers/assets.py

@@ -3,7 +3,8 @@
 from typing import Any, cast
 
 from sift.assets.v1.assets_pb2 import (
-    DeleteAssetRequest,
+    ArchiveAssetRequest,
+    ArchiveAssetResponse,
     GetAssetRequest,
     GetAssetResponse,
     ListAssetsRequest,
@@ -96,6 +97,8 @@ async def update_asset(self, update: AssetUpdate) -> Asset:
         updated_grpc_asset = cast("UpdateAssetResponse", response).asset
         return Asset._from_proto(updated_grpc_asset)
 
-    async def delete_asset(self, asset_id: str, archive_runs: bool = False) -> None:
-        request = DeleteAssetRequest(asset_id=asset_id, archive_runs=archive_runs)
-        await self._grpc_client.get_stub(AssetServiceStub).DeleteAsset(request)
+    async def archive_asset(self, asset_id: str, archive_runs: bool = False) -> list[str] | None:
+        request = ArchiveAssetRequest(asset_id=asset_id, archive_runs=archive_runs)
+        response = await self._grpc_client.get_stub(AssetServiceStub).ArchiveAsset(request)
+        response = cast("ArchiveAssetResponse", response)
+        return response.archived_runs