feat: add video-generation package with OpenAI, Google, and ByteDance providers

[Kamilbenkirane]

Overview

This PR adds the complete video-generation capability package to celeste-python with support for multiple providers.

Package Features

• Unified video generation interface across providers
• Support for OpenAI (Sora 2), Google (Veo 3), and ByteDance (Seedance) models
• Parameter mapping for duration, resolution, aspect_ratio, and image references
• Async polling support for OpenAI video generation jobs
• Type-safe VideoArtifact handling

Configuration Updates

• Add celeste-video-generation to optional dependencies and 'all' extra
• Add video-generation to pytest pythonpath for test discovery
• Update mypy overrides to match private repo pattern (combined block)
• Add video-generation to CI workflow type checking
• Add video-generation to Makefile typecheck target

Integration Tests

• Add integration tests for all three providers
• Use cheapest models with minimum duration and lowest resolution
• Validate VideoGenerationOutput, VideoArtifact, and VideoGenerationUsage

Fixes

• Add type: ignore[override] to streaming.py files for mypy compatibility
• Fix OpenAI integration test to include required aspect_ratio parameter

Testing

• All CI checks pass (linting, formatting, type checking, security, tests)
• 288 tests passing with 85% coverage
• Integration tests ready (require API keys)

[claude]

PR Review: Video Generation Package

Summary

This PR adds comprehensive video generation capabilities to celeste-python with support for OpenAI (Sora 2), Google (Veo 3), and ByteDance (Seedance) providers. The implementation follows established patterns from existing packages and integrates well with the framework architecture.

✅ Strengths

Architecture & Design

• Consistent abstraction: Follows the same client/parameters/models pattern as text-generation and image-generation packages
• Clean separation of concerns: Each provider has well-organized client, config, models, and parameters modules
• Type safety: Strong typing throughout with proper use of TypedDict, Pydantic models, and type annotations
• Extensible design: The ParameterMapper pattern allows elegant handling of provider-specific parameter formats

Code Quality

• Well-documented: Good docstrings explaining provider-specific behaviors (e.g., ByteDance's unique --parameter syntax)
• Error handling: Proper validation and error messages throughout
• Async polling: Clean implementation of async job polling for OpenAI and ByteDance providers
• Configuration management: Provider-specific configs are well-organized and clearly named

Testing & CI Integration

• Integration tests: Comprehensive tests covering all three providers
• CI updates: Properly updated CI workflow, Makefile, and pytest config
• Type checking: Added video-generation to mypy checks with appropriate overrides

🔍 Issues & Recommendations

Critical Issues

1. Missing _parse_finish_reason implementation

Location: packages/video-generation/src/celeste_video_generation/client.py:64

The VideoGenerationClient doesn't implement _parse_finish_reason, which is abstract in other generation clients (text-generation:42, image-generation:42). This inconsistency suggests:

• Either video generation should implement finish reasons, OR
• The base Client class should make this method optional

Recommendation: Check if the base Client class requires this method. If yes, add it with appropriate finish reasons for video generation (e.g., "completed", "failed", "timeout").

2. ByteDance validation inconsistency

Location: packages/video-generation/src/celeste_video_generation/providers/bytedance/client.py:35-75

The _validate_artifacts method converts images to base64 data URIs, but the parameter mappers (lines 142-144, 173-175 in parameters.py) require URLs and raise ValidationError if the artifact doesn't have a URL. This creates a contradiction:

• _validate_artifacts creates data URIs (which ARE URLs starting with "data:")
• But the mappers check if not validated_value.url and raise errors

Impact: This might work because data URIs are URLs, but the error messages are misleading ("ByteDance requires image URL... not base64 data") when base64 data URIs are actually created.

Recommendation: Update error messages to clarify that base64 data URIs are acceptable, or simplify the validation logic.

3. Google download_content is provider-specific

Location: packages/video-generation/src/celeste_video_generation/providers/google/client.py:182-211

Only the Google client implements download_content. This breaks the abstraction if users expect all providers to support this method.

Recommendations:

• Document this as Google-specific behavior in the method docstring
• Consider adding to the base class as an optional method with a default "not supported" implementation
• Or add documentation explaining when/why this is needed for Google but not others

Medium Priority Issues

4. OpenAI image size validation could be more robust

Location: packages/video-generation/src/celeste_video_generation/providers/openai/client.py:120-127

The dimension validation is good, but could be improved - it loads the full image into PIL before validating dimensions, which could be inefficient for large images.

Recommendation: Consider using PIL's lazy loading or reading image dimensions without full decode.

5. Type coercion inconsistency in parameters

Location: Multiple parameter mappers

• OpenAI converts int→string for duration (openai/parameters.py:71-73)
• Google converts string→int for duration (google/parameters.py:66-67)

This is not wrong per se, but it's asymmetric. Users might provide either type, and each provider handles it differently.

Recommendation: Document this flexibility or standardize on one type in the VideoGenerationParameters TypedDict.

6. Error handling in Google parameter mappers could be more specific

Location: packages/video-generation/src/celeste_video_generation/providers/google/parameters.py:104-105, 137-138

When splitting data URIs, the code doesn't catch ValueError from .split(). If a malformed data URI is provided, this will raise an uncaught exception.

Recommendation: Wrap data URI parsing in try/except blocks with proper ValidationError messages.

Minor Issues

7. Magic numbers in configs

Locations:

• bytedance/config.py:12-13: DEFAULT_POLLING_INTERVAL = 5, MAX_POLLING_TIMEOUT = 300
• openai/config.py: POLL_INTERVAL, MAX_POLLS

Recommendation: Consider making these configurable via environment variables or client initialization parameters for flexibility in testing/production.

8. Logging levels could be more granular

Location: Throughout client files

Most logs use logger.info() for operational messages. Consider:

• logger.debug() for polling status checks
• logger.info() for job start/completion
• logger.warning() for retries or degraded service

9. Test parameter type inconsistency

Location: tests/integration_tests/test_video_generation/test_generate.py:14

Tests use string for OpenAI duration and int for others. This is fine given the type coercion, but could be made consistent for clarity.

🔒 Security Considerations

✅ Good Practices

• API keys properly handled via pydantic.SecretStr and .get_secret_value()
• No hardcoded credentials
• Proper use of environment variables for API keys

⚠️ Minor Concerns

• File path handling: When reading files from paths (e.g., openai/client.py:114-115), there's no path validation. Consider validating that paths don't escape intended directories if user input is involved.
• Base64 encoding: Large images encoded to base64 could cause memory issues. Consider adding size limits or warnings.

🚀 Performance Considerations

Strengths

• Async/await used consistently for all I/O operations
• Proper use of asyncio.sleep() for polling
• Efficient polling with reasonable intervals

Suggestions

1. ByteDance polling: Uses fixed 5-second intervals. Consider exponential backoff for long-running jobs.
2. OpenAI polling: Uses for _ in range(MAX_POLLS) which is clear but could benefit from time-based timeout like ByteDance.
3. Image encoding: Base64 encoding happens synchronously. For large images, consider async file I/O or streaming.

📊 Test Coverage

Current State

• Integration tests cover all three providers ✅
• Tests validate Output, Artifact, and Usage types ✅
• Uses cost-effective settings (lowest resolution, minimum duration) ✅

Gaps

• No unit tests for parameter mappers
• No unit tests for client methods (e.g., _validate_artifacts, _parse_content)
• No tests for error conditions (failed jobs, timeouts, malformed responses)
• No tests for edge cases (malformed data URIs, missing files, invalid dimensions)

Recommendation: Add unit tests with mocked HTTP responses to improve coverage from 85% toward 90%+.

🎯 Overall Assessment

Quality Score: 8.5/10

This is a solid, well-architected addition to the codebase. The implementation is consistent with existing patterns, properly typed, and thoroughly integrated with CI/CD. The main issues are:

1. Missing _parse_finish_reason (potential breaking issue)
2. ByteDance validation messaging inconsistency
3. Google's provider-specific download_content method

These are fixable without major refactoring.

✅ Approval Recommendation

APPROVE with minor changes requested

The PR is ready to merge after addressing the critical issues, particularly the _parse_finish_reason implementation. The other issues can be addressed in follow-up PRs if needed.

---

Great work on this implementation! The unified API across providers is elegant and the attention to provider-specific quirks (like ByteDance's --parameter syntax) shows thorough research. 🎉

[claude]

Pull Request Review: Video Generation Package

Overview

This PR successfully adds comprehensive video generation capabilities to celeste-python with support for three major providers (OpenAI Sora 2, Google Veo 3, and ByteDance Seedance). The implementation follows the established patterns from text and image generation packages well.

✅ Strengths

Code Quality & Architecture

• Excellent consistency: The implementation closely follows patterns established in text-generation and image-generation packages
• Type safety: Proper use of TypedDict for parameters, Pydantic models, and type hints throughout
• Provider abstraction: Clean separation of concerns with provider-specific implementations
• Mypy compliance: Appropriate use of type: ignore[override] for known compatibility issues

Implementation Highlights

• Async polling pattern: Well-implemented polling for long-running operations (OpenAI, Google, ByteDance)
• Parameter mapping: Sophisticated handling of provider-specific parameter formats (especially ByteDance's text-embedded parameters)
• Artifact handling: Proper conversion between URLs, base64 data, and file paths for image references
• Error handling: Comprehensive error messages with context

Testing & CI

• Integration tests: Well-structured parametrized tests for all three providers
• Cost-conscious: Uses cheapest models with minimum duration/resolution for testing
• CI integration: Properly added to mypy checks, Makefile, and workflows

🔍 Issues Found

1. Critical: Timeout Not Enforced in ByteDance Client

File: packages/video-generation/src/celeste_video_generation/providers/bytedance/client.py:201-241

The ByteDance client checks elapsed time but doesn't properly enforce the timeout. The while True loop only checks timeout at the start, allowing it to run indefinitely if polling takes time.

Current code:

while True:
    elapsed = time.time() - start_time
    if elapsed > config.MAX_POLLING_TIMEOUT:
        msg = f"ByteDance task {task_id} timed out after {elapsed:.0f}s"
        raise TimeoutError(msg)
    # ... polling logic
    await asyncio.sleep(polling_interval)

Issue: If the last check passes just before timeout, the sleep and subsequent operations can exceed the timeout significantly.

Recommendation: Calculate remaining time and use it for the sleep:

remaining = config.MAX_POLLING_TIMEOUT - elapsed
if remaining <= 0:
    raise TimeoutError(...)
await asyncio.sleep(min(polling_interval, remaining))

2. Bug: OpenAI MAX_POLLS Can Exceed Timeout

File: packages/video-generation/src/celeste_video_generation/providers/openai/client.py:179-204

Using a for-loop with MAX_POLLS iterations is less robust than time-based timeout:

for _ in range(config.MAX_POLLS):
    # ... polling
    await asyncio.sleep(config.POLL_INTERVAL)

This creates a hard limit of polls × interval seconds, but doesn't account for HTTP request latency or API response time.

Recommendation: Use time-based timeout like Google's implementation:

start_time = time.time()
while time.time() - start_time < config.MAX_POLLING_TIMEOUT:
    # polling logic

3. Code Quality: Inconsistent Logging Patterns

Files: All provider clients

• OpenAI uses logger.info(f"Sending request to OpenAI: {request_body}") - logs full request
• Google uses logger.info(f"Initiating video generation with model {model_id}") - logs summary
• ByteDance uses logger.debug("Submitting video generation task to ByteDance") - uses debug level

Recommendation: Standardize logging levels and verbosity:

• Use INFO for operation start/complete (consistent)
• Use DEBUG for detailed request bodies (may contain sensitive data)
• Avoid logging full request bodies at INFO level (OpenAI client)

4. Minor: Redundant File Open Operations

Files:

• packages/video-generation/src/celeste_video_generation/providers/openai/client.py:114
• packages/video-generation/src/celeste_video_generation/providers/google/client.py:46

Both files open images multiple times when handling ImageArtifact with path. The file is read once for validation/conversion, but could be cached.

Impact: Low - only affects path-based artifacts, which are less common than data/url.

5. Security: Base64 Encoding in Logs

File: Multiple files handling image artifacts

When converting images to base64, any logging of these artifacts could expose large amounts of data in logs. While current code seems safe, it's worth noting for monitoring.

Recommendation: Consider adding a size limit or warning for artifact logging.

🚀 Performance Considerations

Strengths

• Async implementation: Proper use of asyncio for non-blocking I/O
• Efficient polling: Reasonable intervals (5s for ByteDance, configurable for others)
• Lazy provider loading: Providers are lazy-loaded in __init__.py

Suggestions

1. Initial wait optimization: ByteDance and Google wait before first poll, but could benefit from exponential backoff instead of fixed intervals
2. HTTP client reuse: The http_client is properly reused across requests (good!)
3. Image caching: Consider caching converted images to avoid repeated base64 encoding

🔒 Security Assessment

✅ Secure Practices

• Proper use of SecretStr for API keys
• No hardcoded credentials
• Environment variable based configuration
• Safe file handling with context managers

⚠️ Minor Concerns

1. File path validation: No validation that ImageArtifact.path points to actual image files before opening
2. Error message exposure: Some error messages include full response data, which might leak sensitive info
3. Timeout as DOS protection: The timeout issues above could theoretically be exploited to tie up resources

📊 Test Coverage

✅ Good Coverage

• Integration tests for all three providers
• Parameterized test design for maintainability
• Tests validate Output, Artifact, and Usage types

🔍 Gaps

1. No unit tests: Only integration tests are present (acceptable for now, but unit tests would help)
2. Error path testing: No tests for API failures, timeouts, or invalid responses
3. Artifact conversion testing: No tests for image-to-base64 conversion edge cases
4. Parameter validation testing: No tests for invalid parameter combinations

Recommendation: Consider adding unit tests for:

• Parameter mappers
• Artifact conversion logic
• Error handling paths
• Timeout scenarios (mocked)

📝 Documentation

✅ Strengths

• Comprehensive README with quick start
• Provider-specific README for ByteDance credentials
• Inline documentation for complex parameter mappings

🔍 Suggestions

• Add docstring examples showing parameter usage
• Document polling behavior and expected wait times
• Add troubleshooting section to README

🎯 Breaking Changes / API Considerations

Good Decisions

1. Version consistency: All packages bumped to 0.2.7 (proper semantic versioning)
2. Backward compatible: New package doesn't affect existing functionality
3. Optional dependency: Properly configured in pyproject.toml

Note

• OpenAI test uses string duration "4" while others use int - this is actually correct per the model constraints and parameter mapper, but could be confusing

📋 Final Recommendations

Must Fix

1. ✅ Fix timeout enforcement in ByteDance client (critical for production reliability)
2. ✅ Fix OpenAI polling to use time-based timeout instead of iteration count

Should Fix

3. Standardize logging patterns across providers
4. Add file path validation before opening files

Nice to Have

5. Add unit tests for core logic
6. Implement exponential backoff for polling
7. Cache converted images to avoid redundant work
8. Add error path integration tests

✅ Verdict

Recommendation: Approve with Minor Changes

This is a high-quality PR that follows the project's established patterns well. The implementation is solid and the integration is clean. The two timeout-related issues should be addressed before merge, but they're straightforward fixes. The code demonstrates good understanding of async Python, type safety, and multi-provider abstraction.

The PR successfully achieves its goals:

• ✅ Complete video generation package
• ✅ Three provider implementations
• ✅ Unified interface
• ✅ Type-safe implementation
• ✅ Proper CI integration
• ✅ Integration tests

Great work! 🎉

---

Reviewed by: Claude (AI Code Reviewer)
Review Date: 2025-11-17

[claude]

PR Review: Video Generation Package

Overview

This is an excellent addition to the celeste-python ecosystem! The implementation demonstrates strong architectural consistency with existing packages (text-generation, image-generation) and provides comprehensive support for three major video generation providers. The code quality is high, with proper type safety, error handling, and documentation.

Strengths

Architecture & Code Quality

• Consistent Design Pattern: Perfectly follows the established pattern from text/image generation packages
• Type Safety: Excellent use of type hints, Pydantic models, and TypedDict for parameters
• Clean Abstractions: Well-designed base VideoGenerationClient
• Parameter Mapping: Sophisticated parameter mapper system handles provider-specific differences elegantly
• Error Handling: Comprehensive error handling with descriptive messages

Provider Implementations

• OpenAI: Clean async polling with multipart form-data support
• Google: Proper base64 encoding and gs:// URI downloads
• ByteDance: Unique text-based parameter embedding handled correctly

Testing & Documentation

• Integration Tests: Good parametrized tests covering all providers
• README: Clear documentation with quick start examples
• CI/CD: Properly integrated into existing workflows

Issues Found

1. CRITICAL - Missing _parse_finish_reason in VideoGenerationClient

The base VideoGenerationClient is missing the _parse_finish_reason abstract method that exists in both ImageGenerationClient and TextGenerationClient.

Location: video-generation/src/celeste_video_generation/client.py:1-65

Impact: Architectural inconsistency. Need to add VideoGenerationFinishReason enum in io.py and implement the method in all providers.

2. Bug - Google convert_to_base64_uri Missing URL Check

Location: google/client.py:42-59

Issue: Doesn't check if img.url already exists. Will fail unnecessarily if user passes ImageArtifact with URL.

Fix: Add URL check like ByteDance does.

3. Missing Timeout - Google Polling

Location: google/client.py:152-174

Issue: Infinite loop without timeout. ByteDance and OpenAI both have timeout protection.

Fix: Add timeout similar to ByteDance.

4. Security - File Path Handling

Locations: openai/client.py:114-117, google/client.py:45-47, bytedance/client.py:48-50

Risk: Reading arbitrary file paths without validation could expose sensitive files.

Fix: Add path validation or document security expectations.

5. Type Inconsistency - Test Duration

Location: tests/integration_tests/test_video_generation/test_generate.py:14

Issue: OpenAI test uses duration as string, others use int.

Fix: Use consistent int type in tests.

6. Minor - ByteDance Parameter Embedding

Location: bytedance/parameters.py:40-49, 83-90

Suggestion: Use next() with generator instead of loop for slight optimization.

Recommendations

Must Fix Before Merge

1. Add _parse_finish_reason to maintain consistency
2. Add timeout to Google polling
3. Fix Google convert_to_base64_uri URL check

Nice to Have

• Unit tests for parameter mappers
• Error case tests (timeouts, failures)
• Path validation documentation
• Exponential backoff for Google polling

Security Assessment: Good

• Proper SecretStr usage
• No hardcoded credentials
• Proper HTTP headers
• Base64 encoding correct
• File path handling needs validation

Test Coverage: Good (85%)

Covered: Integration tests for all providers

Missing: Unit tests for mappers, error paths, edge cases

Conclusion

Recommendation: Approve with requested changes

This is a well-implemented PR with excellent architecture and code quality. The critical issues are:

1. Missing _parse_finish_reason (consistency)
2. Google polling timeout (potential hang)
3. Google URL check (unnecessary failures)

Great work overall!