Test/unit suite acceleration - #2605
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cac6f43d-3de4-41c3-9da9-2171fa9b5831
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cac6f43d-3de4-41c3-9da9-2171fa9b5831
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cac6f43d-3de4-41c3-9da9-2171fa9b5831
There was a problem hiding this comment.
Pull request overview
This PR focuses on accelerating the test/unit suite by refactoring tests to avoid slow subprocess/Engine-driven paths, consolidating parametrized cases, and adding lightweight coverage for newly-adjusted telemetry behavior.
Changes:
- Reworked multiple tests to avoid expensive workflow execution paths (e.g., calling APIs directly, reducing redundant environment setup).
- Simplified/parametrized several test suites to reduce duplication while preserving coverage.
- Updated telemetry logging semantics so cache accounting only occurs when an event is actually submitted, and added unit tests for this behavior.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/workflows/test_setup.py | Avoids CLI subprocess and file I/O by patching requirements generation and calling workflow run API directly. |
| test/telemetry/test_telemetry.py | Adds targeted unit tests for TelemetryLogger submission behavior and Telemetry cache recording. |
| test/telemetry/init.py | Introduces telemetry test package marker file. |
| test/systems/python_environment/test_python_environment_system.py | Reduces venv creation overhead by moving it into the single test that needs it; updates assertions to use system.executable. |
| test/requirements-test-gpu.txt | Adds slicegpt to GPU test requirements. |
| test/passes/qairt/test_preparation.py | Strengthens cleanup assertion by patching Path.unlink and asserting it’s invoked. |
| test/passes/pytorch/test_slicegpt.py | Re-enables SliceGPT test under Python 3.10+ + GPU (removes unconditional skip). |
| test/passes/pytorch/test_selective_mixed_precision.py | Consolidates duplicate tests via parametrization (group_size/high_group_size cases; KLD modes). |
| test/passes/pytorch/test_lora.py | Removes unconditional skip from GPU-dependent LoRA tests. |
| test/passes/onnx/test_transformer_optimization.py | Splits invalid-config validation from a single “valid CUDA config runs” execution test. |
| test/passes/onnx/test_qairt_mha2sha.py | Removes a redundant fallback test and clarifies fallback configuration comments. |
| test/passes/onnx/test_peephole_optimizer.py | Avoids helper conversion path by saving ONNX and using ONNXModelHandler directly; merges optimizer tests. |
| test/passes/onnx/test_model_builder.py | Makes layer-annotation test lightweight via a mocked GenAI builder and explicit node metadata assertions. |
| test/passes/onnx/test_mobius_model_builder.py | Drops redundant “model.onnx exists after run” test. |
| test/passes/onnx/test_mnb_to_qdq.py | Makes model fixture module-scoped and improves exception handling around older ORT behavior. |
| test/passes/onnx/test_extract_adapters.py | Clarifies “no adapters” behavior via an explicit test and removes None model_type branch. |
| test/passes/onnx/test_common.py | Avoids heavy conversion dependency by constructing ONNX models directly (inline vs external-data paths). |
| test/passes/onnx/test_add_metadata.py | Refactors/parametrizes metadata tests and relocates model-hash consistency coverage to model tests. |
| test/model/test_onnx_model.py | Adds a direct test that model identifier generation is consistent and SHA256-length. |
| test/engine/packaging/test_packaging_generator.py | Switches packaging tests to use generate_output_artifacts directly and adds helpers for workflow output + metrics. |
| test/common/quant/test_utils.py | Collapses multiple range tests into one parametrized test; removes redundant shape-trimming test. |
| test/common/quant/test_nn.py | Parametrizes initialization coverage for QuantLinear/QuantEmbedding. |
| test/common/quant/test_hf_utils.py | Parametrizes HF quant config initialization tests to reduce duplication. |
| test/cli/test_run_pass.py | Avoids subprocess invocation by exercising argparse registration/help directly. |
| test/cli/test_cli.py | Exercises CLI help/errors via in-process cli_main, and keeps a targeted subprocess entrypoint check. |
| olive/telemetry/telemetry.py | Records cached “event logged” only when logger reports submission. |
| olive/telemetry/library/telemetry_logger.py | Changes TelemetryLogger.log to return bool indicating whether the event was submitted; respects disabled logger state. |
Suppressed comments (1)
test/engine/packaging/test_packaging_generator.py:102
- With the current cleanup,
shutil.rmtree("mlruns")will remove whatevermlrunsdirectory exists in the current working directory. After switching to atmp_pathworking directory for the MLflow-export branch, clean up the directory explicitly undertmp_pathso the deletion target is unambiguous.
if export_in_mlflow_format:
assert (candidate_model_path / "mlflow_model").exists()
if Path("mlruns").exists():
shutil.rmtree("mlruns")
| expected_command = [ | ||
| python_path, | ||
| self.system.executable, | ||
| str(script_path), | ||
| "--dummy_config", | ||
| str(tmp_path / "dummy_config.json"), |
| def test_generate_zipfile_artifacts_without_metrics(tmp_path, export_in_mlflow_format): | ||
| packaging_config = PackagingConfig( | ||
| type=PackagingType.Zipfile, | ||
| name="OutputModels", | ||
| config=ZipfilePackagingConfig(export_in_mlflow_format=export_in_mlflow_format), |
| -r requirements-test.txt | ||
| bitsandbytes | ||
| onnxruntime-genai-cuda | ||
| slicegpt |
There was a problem hiding this comment.
is this required because the slicegpt pass test is not skipped anymore. concerned that the package might be old and it installs an older version of transformers because of this. can you check the important dependencies like transformers and others don't get affected by this added dependency?
There was a problem hiding this comment.
should we remove this pass? do you know any vendor needs this?
There was a problem hiding this comment.
i am not familiar with any users for this pass but i think it is still a relevant pass? shaahji might have more thoughts and insight on the test dependencies
Co-authored-by: xiaoyu-work <85524621+xiaoyu-work@users.noreply.github.com>
|
Copilot resolve the merge conflicts in this pull request |
…eration # Conflicts: # test/passes/onnx/test_mnb_to_qdq.py Co-authored-by: xiaoyu-work <85524621+xiaoyu-work@users.noreply.github.com>
|
Copilot resolve the merge conflicts in this pull request |
…eration # Conflicts: # test/common/quant/test_nn.py # test/passes/onnx/test_common.py Co-authored-by: xiaoyu-work <85524621+xiaoyu-work@users.noreply.github.com>
Merged
|
Describe your changes
Test/unit suite acceleration
Checklist before requesting a review
lintrunner -a(Optional) Issue link