test: add v2 regression guards and supersession markers#1119
test: add v2 regression guards and supersession markers#1119ajcasagrande wants to merge 1 commit into
Conversation
Signed-off-by: Anthony Casagrande <acasagrande@nvidia.com>
Try out this PRQuick install: pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@c7e1a7b7dcb83cd037ce71677e95158fb2809521Recommended with virtual environment (using uv): uv venv --python 3.12 && source .venv/bin/activate
uv pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@c7e1a7b7dcb83cd037ce71677e95158fb2809521Last updated for commit: |
WalkthroughThis PR adds test-only changes: a default environment variable for MLflow file-store support in integration test setup, and three new/updated unit test modules that verify config loader v1 removal, dataset model prerequisite handling, and orchestrator v1 execution method removal. ChangesTest suite additions
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/common/test_dataset_models_prereq.py (1)
7-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing return type hints on test functions.
All four test functions lack
-> Noneannotations, unlike the sibling guard-test files in this PR.Based on coding guidelines, which state: "Add type hints on ALL functions (params and return types)".✏️ Proposed fix
-def test_turn_defaults_empty_prerequisites(): +def test_turn_defaults_empty_prerequisites() -> None: t = Turn() assert t.prerequisites == [] -def test_turn_carries_prerequisites(): +def test_turn_carries_prerequisites() -> None: p = TurnPrerequisite(kind=PrerequisiteKind.SPAWN_JOIN, branch_id="b1") t = Turn(prerequisites=[p]) assert len(t.prerequisites) == 1 assert t.prerequisites[0].branch_id == "b1" -def test_turn_metadata_carries_prerequisites(): +def test_turn_metadata_carries_prerequisites() -> None: p = TurnPrerequisite(kind=PrerequisiteKind.SPAWN_JOIN, branch_id="b1") m = TurnMetadata(prerequisites=[p]) assert m.prerequisites == [p] -def test_turn_metadata_copied_from_turn(): +def test_turn_metadata_copied_from_turn() -> None: p = TurnPrerequisite(kind=PrerequisiteKind.SPAWN_JOIN, branch_id="b1") t = Turn(prerequisites=[p], branch_ids=["b1"]) m = t.metadata() assert m.prerequisites == [p] assert m.branch_ids == ["b1"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/common/test_dataset_models_prereq.py` around lines 7 - 30, All four test functions in test_dataset_models_prereq.py are missing explicit return type annotations. Update test_turn_defaults_empty_prerequisites, test_turn_carries_prerequisites, test_turn_metadata_carries_prerequisites, and test_turn_metadata_copied_from_turn to use -> None so they match the typing style used in the sibling test files and comply with the function type-hint guideline.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/common/test_dataset_models_prereq.py`:
- Around line 7-30: All four test functions in test_dataset_models_prereq.py are
missing explicit return type annotations. Update
test_turn_defaults_empty_prerequisites, test_turn_carries_prerequisites,
test_turn_metadata_carries_prerequisites, and
test_turn_metadata_copied_from_turn to use -> None so they match the typing
style used in the sibling test files and comply with the function type-hint
guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 49a2e831-acfc-4f37-ba05-57d61affb4fe
📒 Files selected for processing (4)
tests/integration/conftest.pytests/unit/common/config/test_loader.pytests/unit/common/test_dataset_models_prereq.pytests/unit/orchestrator/test_orchestrator_execution.py
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Test-only carve from the agentx branch: three regression guards plus one integration-conftest env hunk. No
src/changes; every imported symbol already exists onmain.What each guard pins
tests/unit/common/test_dataset_models_prereq.py(new, 4 tests) — pins theTurn/TurnMetadataprerequisite plumbing: default-emptyprerequisites,TurnPrerequisite(kind=PrerequisiteKind.SPAWN_JOIN, ...)carried on both models, andTurn.metadata()copyingprerequisites+branch_idsthrough. Imports onlyaiperf.common.enums.PrerequisiteKindandaiperf.common.models.{Turn, TurnMetadata, TurnPrerequisite}.tests/unit/orchestrator/test_orchestrator_execution.py(new, 1 test) — supersession marker for the v1 orchestrator execution suite removed by feat: multi-tier SLO search — resolve N tier boundaries in one job #1035. Asserts the v1 methods (_resolve_strategy,_execute,_execute_loop,_create_sweep_strategy,_create_confidence_strategy) stay absent fromMultiRunOrchestrator, and that the v2 replacements (MultiRunOrchestrator.execute,aiperf.cli_runner._strategy.build_strategy) exist. The module docstring maps every v1 test concern to its v2 coverage so a re-port is forced (not silently skipped) if the v1 model ever returns.tests/unit/common/config/test_loader.py(new, 1 test) — supersession marker for the removed v1aiperf.common.config.loader. Asserts the module stays gone (importlibonly, no src dependency) and documents where each v1 loader behavior is now covered (tests/unit/config/test_loader_edge_cases.py/test_loader_adversarial.py/test_end_to_end_config_flow.py).tests/integration/conftest.py(+7) —os.environ.setdefault("MLFLOW_ALLOW_FILE_STORE", "true"). MLflow 3.x raises onfile://tracking stores unless this opt-out is set; the MLflow integration tests (tests/integration/post_processors/test_mlflow_live_correctness.py,tests/integration/plot/test_plot_mlflow_upload.py) usefile://stores undertmp_path.setdefaultso explicit overrides win.Evidence
uv run pytest -n auto tests/unit/common/test_dataset_models_prereq.py tests/unit/orchestrator/test_orchestrator_execution.py tests/unit/common/config/test_loader.py— 6 passed.uv run pytest --collect-only -q tests/unit/— 13874 collected, no errors.ruff format+ruff checkon the four touched files — clean.make check-ruff-baselined— OK (0 new);make check-ergonomics— OK (0 new).main(031407401):PrerequisiteKind.SPAWN_JOIN,Turn/TurnMetadataprerequisites+branch_idsfields andTurn.metadata(),MultiRunOrchestrator.execute,cli_runner._strategy.build_strategy, andaiperf.common.configabsent fromsrc/.🤖 Generated with Claude Code
Summary by CodeRabbit