Skip to content
This repository was archived by the owner on Apr 15, 2026. It is now read-only.

Mutable defaults fixes#222

Merged
tempusfrangit merged 2 commits into
mainfrom
mutable-defaults
Sep 24, 2025
Merged

Mutable defaults fixes#222
tempusfrangit merged 2 commits into
mainfrom
mutable-defaults

Conversation

@tempusfrangit
Copy link
Copy Markdown
Contributor

@tempusfrangit tempusfrangit commented Sep 22, 2025

Python's mutable default footgun (shared mutable objects across function calls)
was not being caught in Cog runtime, leading to subtle bugs where predictors with
default=[] would share the same list instance across predictions.

Replace error-throwing validation with automatic conversion of mutable defaults
to safe default_factory implementations. This prevents the footgun while
maintaining backward compatibility and requiring no code changes.

Automatic Conversion Logic:

  • Empty collections: []default_factory=list, {}default_factory=dict
  • Populated collections: [1,2,3]default_factory=lambda: copy.deepcopy([1,2,3])
  • Custom objects: MyObj()default_factory=lambda: copy.deepcopy(MyObj())
  • Immutable types: No conversion needed (strings, numbers, tuples, etc.)

Collection Type Support:

  • Lists: list, List[T], list[T] (Python 3.9+)
  • Dicts: dict, Dict[K,V], dict[K,V] (Python 3.9+)
  • Sets: set, Set[T], set[T] (Python 3.9+)
  • Custom objects: Any mutable object not in allowlist

Implementation:

  • Use real dataclass.Field objects with proper default_factory
  • Automatic conversion happens at import time in Input() function
  • Runtime properly executes factories to create fresh instances per prediction
  • Full isolation verified with E2E tests

Testing & Validation:

  • Updated all tests to verify auto-conversion instead of error cases
  • New E2E test verifies no state leakage between predictions
  • Python 3.9-3.13 compatibility maintained
  • Go integration tests updated for new behavior

Before (silent footgun):

def predict(self, items: List[str] = Input(default=[])) -> str:
    items.append("new")  # Modifies shared list across predictions!
    return str(items)

After (automatic fix):

  def predict(self, items: List[str] = Input(default=[])) -> str:
      items.append("new")  # Safe: auto-converted to fresh list each time
      return str(items)   # No code changes required!

Prevents mutable default footgun with zero breaking changes to existing code.

This accurately describes what we actually implemented - automatic conversion rather than validation errors.

@tempusfrangit tempusfrangit requested a review from a team as a code owner September 22, 2025 23:28
Copy link
Copy Markdown

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 implements comprehensive mutable defaults validation for the Cog runtime to prevent Python's mutable default footgun where shared mutable objects across function calls can cause subtle bugs. The implementation adds import-time validation that hard errors on mutable defaults and suggests proper default_factory usage, with comprehensive support for all collection types in both Python 3.9+ and typing module syntax.

  • Added comprehensive mutable defaults validation to the Input() function with clear error messages
  • Enhanced type system to support all collection types including sets and bare types like listList[Any]
  • Implemented proper default_factory support using real dataclass.Field objects
  • Fixed compatibility issues across Python versions 3.9-3.13

Reviewed Changes

Copilot reviewed 30 out of 32 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
python/coglet/api.py Added mutable default validation and default_factory parameter to Input() function
python/coglet/adt.py Enhanced type system for collection types and added PrimitiveType.ANY
python/coglet/schemas.py Fixed Secret schema encoding with proper Field object handling
python/coglet/inspector.py Enhanced Field object handling in validation and runtime execution
python/cog/coder/set_coder.py Added new SetCoder for set type support
python/cog/coder/json_coder.py Enhanced JsonCoder for Python 3.13 compatibility
python/cog/coder/pydantic_coder.py Fixed BaseModelCoder for generic type compatibility
python/tests/test_input_function.py Added comprehensive mutable defaults validation tests
python/tests/test_input_functional.py Updated test to use default_factory instead of mutable default
python/tests/schemas/*.py Updated all schema test files to use default_factory for mutable defaults
python/tests/runners/*.py Added collection type test files and updated existing ones
python/tests/cases/repetition.py Updated to use default_factory for list default
python/tests/bad_predictors/*.py Updated test files to use default_factory for mutable defaults
internal/tests/input_defaults_test.go Added Go integration tests for mutable default validation

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread python/coglet/inspector.py Outdated
Comment thread python/coglet/inspector.py Outdated
Comment thread python/coglet/inspector.py Outdated
@tempusfrangit tempusfrangit force-pushed the mutable-defaults branch 2 times, most recently from c0f913a to 811ee48 Compare September 22, 2025 23:31
@tempusfrangit tempusfrangit changed the title Add comprehensive mutable defaults validation with full collection type support Mutable defaults validation Sep 22, 2025
Python's mutable default footgun (shared mutable objects across function calls)
was not being caught in Cog runtime, leading to subtle bugs where predictors
with `default=[]` would share the same list instance across predictions.

Implement import-time validation that hard errors on mutable defaults and
suggests proper `default_factory` usage, with comprehensive support for all
collection types in both Python 3.9+ and typing module syntax.

- Detect mutable defaults (`[]`, `{}`, `set()`, custom objects) at import time
- Provide clear, actionable error messages:
- `default=[]` → "Use Input(default_factory=list) instead"
- `default=[1,2,3]` → "Use Input(default_factory=lambda: [1,2,3]) instead"
- Validate mutual exclusion of `default` and `default_factory` parameters

- **Lists**: `list`, `List[T]`, `list[T]` (Python 3.9+)
- **Dicts**: `dict`, `Dict[K,V]`, `dict[K,V]` (Python 3.9+)
- **Sets**: `set`, `Set[T]`, `set[T]` (Python 3.9+)
- **Bare types**: `list`→`List[Any]`, `dict`→`Dict[str,Any]`, `set`→`Set[Any]`

- Use real `dataclass.Field` objects instead of bespoke implementation
- Compatible with Python 3.9+ (`kw_only` parameter handling)
- Full support for `default_factory` execution at runtime

- Go integration tests verify end-to-end runtime behavior
- Python tests validate all collection type combinations
- Smart pytest skipping for intentionally-bad test modules

- `python/coglet/api.py`: Added mutable default validation to `Input()` function
- `python/coglet/adt.py`: Enhanced type system for all collection types, added `PrimitiveType.ANY`
- `python/coglet/schemas.py`: Fixed Secret schema encoding with proper normalization
- `python/coglet/inspector.py`: Enhanced Field object handling in validation/runtime
- `python/cog/coder/`: Added `SetCoder`, enhanced `JsonCoder` for Python 3.13 compatibility

- 10 mutable defaults validation tests (empty/populated collections, custom objects)
- 9 collection type tests (typing vs built-in vs bare syntax)
- Go integration tests for setup failure detection
- Python 3.9, 3.10, 3.11, 3.12, 3.13 compatibility validation

- Fixed `Field` constructor for Python 3.9 (`kw_only` parameter)
- Fixed `issubclass()` and `inspect.getmro()` for generic types in Python 3.13
- Enhanced `pydantic_coder` and `json_coder` for generic type compatibility

Before (silent footgun):
```python
def predict(self, items: List[str] = Input(default=[])) -> str:
    items.append("new")  # Modifies shared list across predictions!
    return str(items)
```

After (clear error + fix):
```python
def predict(self, items: List[str] = Input(default_factory=list)) -> str:
    items.append("new")  # Safe: fresh list each time
    return str(items)
```
Testing

- Python tests: 19/19 collection + validation tests
- Go tests: 3/3 integration tests
- mypy: All type checking passes
- Multi-version: Python 3.9, 3.10, 3.11, 3.12, 3.13 validated

Fixes mutable default footgun while maintaining full backward compatibility
for correctly-written predictors.
Copilot AI review requested due to automatic review settings September 22, 2025 23:45
Copy link
Copy Markdown

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

Copilot reviewed 30 out of 32 changed files in this pull request and generated 2 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread python/coglet/api.py Outdated
Comment thread python/coglet/schemas.py
Copy link
Copy Markdown
Contributor

@michaeldwan michaeldwan left a comment

Choose a reason for hiding this comment

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

Code good, but be warned, I have less than zero experience with this aspect of python 😬

Let's talk about the question I raised about failing vs best-effort correcting.

Comment thread python/coglet/api.py Outdated
Replace error-throwing validation with automatic conversion of mutable defaults
to safe default_factory implementations. This prevents Python's mutable default
footgun while maintaining backward compatibility.

Changes:
- Convert empty collections ([], {}, set()) to built-in constructors (list, dict, set)
- Convert populated collections/objects to lambda factories using copy.deepcopy
- Update all tests to verify auto-conversion instead of error cases
- Add comprehensive E2E test verifying isolation between prediction calls
- Remove pytest skip logic from test files now that conversion works
Copy link
Copy Markdown

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

Copilot reviewed 31 out of 33 changed files in this pull request and generated 2 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread python/tests/test_input_function.py
Comment thread python/coglet/api.py
@tempusfrangit tempusfrangit changed the title Mutable defaults validation Mutable defaults fixes Sep 24, 2025
Copy link
Copy Markdown
Contributor

@michaeldwan michaeldwan left a comment

Choose a reason for hiding this comment

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

looks good, I like the new behavior!

@tempusfrangit tempusfrangit merged commit b10d682 into main Sep 24, 2025
23 checks passed
@tempusfrangit tempusfrangit deleted the mutable-defaults branch September 24, 2025 17:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants