[feat] [1/n] API improvements: add intial files for new fastvideo public API#1218
[feat] [1/n] API improvements: add intial files for new fastvideo public API#1218SolitaryThinker merged 6 commits intomainfrom
Conversation
|
/test full |
mergify
bot
added
type: feat
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🔴 PR merge requirementsThis rule is failing.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a typed configuration system for the FastVideo inference API, replacing legacy argument handling with a structured, dataclass-based schema. The implementation includes a recursive schema parser in fastvideo/api/parser.py that supports nested dataclasses, unions, and literals, along with a mechanism for applying dotted-key overrides from the CLI. Additionally, a comprehensive schema parity inventory and a suite of tests are provided to ensure consistency during the migration. The review feedback highlights opportunities to improve the parser's robustness, specifically by enhancing error reporting for Union types and adding explicit support for Enum types in both general values and dictionary keys.
| expected = ", ".join(_type_name(candidate) for candidate in candidates) | ||
| detail = errors[0] if errors else f"expected one of ({expected})" | ||
| raise ConfigValidationError(path, detail) |
There was a problem hiding this comment.
When parsing a Union, if all candidates fail, the current implementation only returns the error message from the first candidate. This can be misleading for users. For example, if the type is int | list[int] and the user provides a list with an invalid element, they might see an error saying 'expected int' instead of a more specific error about the list element. Consider aggregating errors or providing a more descriptive 'expected one of' message that includes the specific failure if only one candidate of the same base type (e.g., a collection) was attempted.
| def _parse_instance(self, annotation: Any, value: Any, path: str) -> Any: | ||
| if isinstance(annotation, type) and not isinstance(value, annotation): | ||
| raise ConfigValidationError(path, f"expected {annotation.__name__}") | ||
| return value |
There was a problem hiding this comment.
The _parse_instance method and the general parser currently lack explicit support for Enum types. If a dataclass field is an Enum, isinstance(value, annotation) will fail when value is a raw string or integer from the configuration file. Although the current schema uses Literal to avoid this, adding Enum support would improve the robustness and extensibility of the API config layer.
| def _parse_dict_key(self, annotation: Any, value: Any, path: str) -> Any: | ||
| if annotation is Any: | ||
| return value | ||
| if annotation is str: | ||
| if not isinstance(value, str): | ||
| raise ConfigValidationError(path, "expected string dictionary keys") | ||
| return value | ||
| if annotation is int: | ||
| if not isinstance(value, int) or isinstance(value, bool): | ||
| raise ConfigValidationError(path, "expected integer dictionary keys") | ||
| return value | ||
| return value |
There was a problem hiding this comment.
The _parse_dict_key method only explicitly validates str and int keys. If a dictionary uses a different key type (like an Enum), it falls through and returns the raw value without validation or conversion. This could lead to type mismatches in the resulting dataclass if the key requires instantiation from a string.
1 participant
Purpose
This PR lays the groundwork for the inference API refactor without changing runtime behavior yet.
docs/design/inference_schema_parity_inventory.yamlfastvideo/apifastvideo/tests/apiThis PR does not yet hook the new schema into VideoGenerator, CLI runtime entrypoints, or the server path. It is a boundary-definition and test-foundation PR.
Changes
The current inference surface mixes init-time config, request-time sampling, and model-specific knobs across flat kwargs and flags. This PR creates a typed, testable
boundary so the later runtime migration can happen with explicit parity checks instead of ad hoc argument churn.
Test Plan
/home/william5lin/miniconda3/envs/fv-main/bin/python -m pytest -q fastvideo/tests/api
Test Results
Test output
Checklist
pre-commit run --all-filesand fixed all issuesFor model/pipeline changes, also check: