Skip to content

feat: add 4k support and artifact base64 serialization#106

Closed
Kamilbenkirane wants to merge 5 commits into
mainfrom
feat/veo-4k-and-artifact-serialization
Closed

feat: add 4k support and artifact base64 serialization#106
Kamilbenkirane wants to merge 5 commits into
mainfrom
feat/veo-4k-and-artifact-serialization

Conversation

@Kamilbenkirane

Copy link
Copy Markdown
Member

This PR adds 4k support for Veo 3.1 and implements base64 serialization for Artifact data to ensure JSON compatibility.

- client.py: "...API client with shared implementation." → "...API client mixin."
- config.py: "Endpoints for X API." → "Endpoints for Provider X API."
- parameters.py: Minor naming consistency
…arameters

- Add WEB_SEARCH, X_SEARCH, CODE_EXECUTION to TextGenerationParameter
- Google: web_search support
- OpenAI: web_search support
- xAI: web_search, x_search, code_execution support
- Fix THINKING_LEVEL → THINKING_BUDGET for grok-3-mini model
- Add WEB_SEARCH constraint to all Claude models
- Implement WebSearchMapper using web_search_20250305 tool type
- Add WEB_SEARCH constraint to all Mistral models
- Implement WebSearchMapper using brave_web_search connector
- Fix Google generate_content docstrings for consistency
@claude

claude Bot commented Jan 14, 2026

Copy link
Copy Markdown

Pull Request Review: feat: add 4k support and artifact base64 serialization

Summary

This PR adds two main features:

  1. 4K video support for Veo 3.1 models
  2. Base64 serialization for Artifact data to ensure JSON compatibility
  3. Web search, X search, and code execution parameters across multiple providers
  4. Bug fix: Corrects THINKING_LEVELTHINKING_BUDGET for grok-3-mini

✅ Code Quality & Best Practices

Strengths

  1. Consistent Pattern Application: The web_search parameter follows the established mapper pattern consistently across all providers (Anthropic, Google, Mistral, OpenAI, xAI)
  2. Clean Architecture: Each provider implements its own mapper subclass, maintaining separation of concerns
  3. Type Safety: Proper use of Bool() constraints for boolean parameters
  4. Docstring Updates: Good cleanup of docstrings for consistency (e.g., "API client mixin" changes)

Areas for Improvement

1. Missing Test Coverage ⚠️

Issue: No tests found for the new features:

  • No tests for base64 serialization of Artifact.data (src/celeste/artifacts.py:28-33)
  • No tests for web_search, x_search, or code_execution parameters
  • No tests for 4K resolution support

Recommendation: Add tests covering:

# Test base64 serialization
def test_artifact_data_serialization():
    artifact = Artifact(data=b"test data")
    serialized = artifact.model_dump()
    assert serialized["data"] == "dGVzdCBkYXRh"  # base64 encoded
    
# Test web_search parameter mapping
def test_anthropic_web_search_mapper():
    # Verify the tools field is correctly populated
    ...

2. Inconsistent Web Search Implementation 🔍

Issue: Different providers use different tool formats:

  • Anthropic: {"type": "web_search_20250305", "name": "web_search"} (packages/providers/anthropic/src/celeste_anthropic/messages/parameters.py:144-147)
  • Google: [{"google_search": {}}] (packages/providers/google/src/celeste_google/generate_content/parameters.py:207)
  • Mistral: {"type": "web_search"} (packages/providers/mistral/src/celeste_mistral/chat/parameters.py:64)
  • OpenAI: {"type": "web_search"} (packages/providers/openai/src/celeste_openai/responses/parameters.py:100)
  • xAI: {"type": "web_search"} (packages/providers/xai/src/celeste_xai/responses/parameters.py:82)

Question: Are these provider-specific formats correct according to their APIs? The variation suggests they might be, but it would be good to confirm with API documentation or add comments explaining the differences.

3. Missing Validation for Mutually Exclusive Tools 🔧

Issue: The mappers use setdefault("tools", []).append() which could lead to duplicate tool definitions if called multiple times.

Example (packages/providers/xai/src/celeste_xai/responses/parameters.py):

request.setdefault("tools", []).append({"type": "web_search"})  # Line 82
request.setdefault("tools", []).append({"type": "x_search"})    # Line 100
request.setdefault("tools", []).append({"type": "code_execution"})  # Line 118

Recommendation: Add duplicate prevention:

tools = request.setdefault("tools", [])
tool_entry = {"type": "web_search"}
if tool_entry not in tools:
    tools.append(tool_entry)

4. Bug Fix Verification ✓

Issue: The PR description mentions fixing THINKING_LEVELTHINKING_BUDGET for grok-3-mini, which I can confirm in the diff:

-TextGenerationParameter.THINKING_LEVEL: Choice(options=["low", "high"]),
+TextGenerationParameter.THINKING_BUDGET: Choice(options=["low", "high"]),

Question: This is a breaking change. Are there any existing users relying on THINKING_LEVEL for grok-3-mini that need migration guidance?

🔒 Security Considerations

Base64 Serialization Implementation ✅

The base64 serialization (src/celeste/artifacts.py:28-33) looks secure:

@field_serializer("data")
def serialize_data(self, value: bytes | None) -> str | None:
    if value is None:
        return None
    return base64.b64encode(value).decode("ascii")

Strengths:

  • Properly handles None values
  • Uses standard library base64 module
  • Decodes to ASCII for JSON compatibility

Consideration:

  • No deserialization method is provided. If users need to reconstruct Artifact objects from JSON, they'll need to manually decode the base64 string. Consider adding a validator or custom deserialization if this is a common use case.

⚡ Performance Considerations

Base64 Encoding Overhead 📊

Issue: Base64 encoding increases data size by ~33%. For large artifacts (images, videos, audio), this could be significant.

Recommendation:

  1. Document the size impact in docstrings
  2. Consider adding a warning if data exceeds a certain threshold (e.g., 10MB)
  3. Suggest users prefer url or path for large files

Multiple Tool Additions 🔄

The current implementation calls setdefault() and append() for each tool, which could be optimized if multiple tools are commonly used together.

🐛 Potential Issues

1. Missing Veo 3.1 Parameter Validation

Issue: Adding "4k" to the resolution choices (packages/capabilities/video-generation/src/celeste_video_generation/providers/google/models.py:48,68) without validating:

  • Is 4K supported for all duration options (4, 6, 8 seconds)?
  • Are there any model-specific constraints or higher costs?

Recommendation: Add a comment or documentation about any limitations.

2. Google WebSearch Override Behavior

Issue: Google's WebSearchMapper completely replaces the tools array:

request["tools"] = [{"google_search": {}}]  # Line 207

This differs from other providers that append to the array. This could overwrite user-provided tools.

Recommendation: Use the same pattern as other providers:

request.setdefault("tools", []).append({"google_search": {}})

📝 Documentation Gaps

  1. No changelog or migration notes for the THINKING_LEVELTHINKING_BUDGET breaking change
  2. No examples of how to use the new web_search, x_search, or code_execution parameters
  3. No documentation about base64 serialization behavior in Artifact class

🎯 Recommendations Summary

High Priority

  1. Add tests for all new functionality
  2. ⚠️ Fix Google WebSearch to not override existing tools
  3. 📚 Add documentation for breaking changes and new parameters

Medium Priority

  1. 🔍 Add duplicate prevention in tool mappers
  2. 📊 Document base64 performance implications
  3. 🔄 Add deserialization support for base64 data if needed

Low Priority

  1. 💬 Add comments explaining provider-specific tool formats
  2. Consider size warnings for large artifact data

✨ Overall Assessment

This is a solid PR that adds valuable functionality with good architectural consistency. The main concerns are:

  • Missing test coverage (critical)
  • Google WebSearch override behavior (bug)
  • Documentation gaps (important)

The code follows established patterns well and the base64 serialization implementation is clean and secure. With tests and the Google tools fix, this would be ready to merge.

Recommended Action: Request changes for tests and the Google tools bug, then approve.

@Kamilbenkirane Kamilbenkirane deleted the feat/veo-4k-and-artifact-serialization branch January 14, 2026 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant