Deprecate video feature and unify legacy wrapper (app.py as canonical)#24
Open
SaltProphet wants to merge 1 commit intomainfrom
Open
Deprecate video feature and unify legacy wrapper (app.py as canonical)#24SaltProphet wants to merge 1 commit intomainfrom
SaltProphet wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request deprecates the video rendering feature and consolidates the legacy wrapper module to eliminate code duplication. The changes establish app.py as the single canonical source of truth by converting forgev1.py from a ~190-line duplicate implementation into a thin ~55-line compatibility re-export module. Video functionality is formally deprecated by removing configuration constants, directory setup, documentation references, and the render_video import from the REST API, while maintaining a deprecated stub that raises NotImplementedError for backward compatibility.
Changes:
- Removed video feature configuration (VIDEO_FPS, VIDEO_BITRATE, ASPECT_RATIOS), the output/videos directory from FORGEConfig, and video references from module docstrings
- Refactored forgev1.py to re-export all functions from app.py instead of maintaining duplicate implementations, preserving the same public API surface
- Updated tests to remove output/videos directory expectations and video benchmark references, and added README deprecation notice
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| app.py | Removed video configuration constants (VIDEO_FPS, VIDEO_BITRATE, ASPECT_RATIOS) and output/videos directory from FORGEConfig.DIRECTORIES; kept render_video stub raising NotImplementedError |
| forgev1.py | Replaced ~190 lines of duplicated wrapper logic with thin re-export module (~55 lines) importing all public functions from app.py |
| api.py | Removed render_video import and video rendering from REST API documentation |
| README.md | Added deprecation notice under REST API section stating "Video export is deprecated and no longer supported" |
| tests/conftest.py | Removed output/videos from test directory setup fixture |
| tests/integration/test_workflows.py | Removed output/videos from required directory validation test |
| tests/integration/test_speed_benchmarks.py | Removed "Video rendering" from benchmark feature list comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
app.pyis the single source of truth.Description
render_videoimport fromapi.pyand trimmed the REST API docs to stop advertising video support.output/videosdirectory fromFORGEConfiginapp.py, while keeping a deprecatedrender_videowrapper that raisesNotImplementedError.forgev1.pywith a thin compatibility re-export module that delegates toapp.pyto eliminate duplicated wrapper logic and reduce drift.output/videosexpectations from fixtures and integration/benchmark descriptions.Testing
pytest -q -o addopts='' tests/integration/test_workflows.py::TestDirectoryManagement::test_directory_creation tests/unit/test_utils.py, and the selected tests passed (10 passed in 3.83s).python -m py_compile api.py app.py forgev1.py, which succeeded with no syntax errors.Deprecate video feature canonically and align wrappersand validated the edited files (api.py,app.py,forgev1.py,README.md,tests/conftest.py,tests/integration/test_speed_benchmarks.py,tests/integration/test_workflows.py).Codex Task