Skip to content

extend command line --test to trigger speedup measure#2536

Open
xadupre wants to merge 7 commits into
mainfrom
xadupre/cmd
Open

extend command line --test to trigger speedup measure#2536
xadupre wants to merge 7 commits into
mainfrom
xadupre/cmd

Conversation

@xadupre

@xadupre xadupre commented Jun 22, 2026

Copy link
Copy Markdown
Member

Describe your changes

--test only enables discrepancy measure by default, it needs to be extended to let the user add speedup measures

@xadupre

xadupre commented Jun 22, 2026

Copy link
Copy Markdown
Member Author

@copilot can you documentation the option somewhere

Copilot AI commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

@copilot can you documentation the option somewhere

Added documentation for the new --test_metrics option (including speedup) in docs/source/how-to/cli/cli-fast-test.md in commit 15287d8.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends Olive’s CLI --test workflow by allowing callers to choose which discrepancy-check metrics are evaluated (accuracy via MAE and/or speedup via latency timing), and adds coverage + documentation for the new option.

Changes:

  • Add --test_metrics CLI option (choices: mae, speedup) and plumb it through olive run and the base CLI workflow runner.
  • Update discrepancy-check pass injection to enable/disable MAE threshold enforcement and speedup timing based on selected metrics.
  • Add unit tests for metric selection behavior and warning when --test_metrics is provided without --test, plus docs describing usage.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
test/cli/test_base.py Adds tests for metric-selection behavior and unused-flag warning behavior.
olive/cli/run.py Passes test_metrics into discrepancy-check injection and warns when --test_metrics is unused.
olive/cli/base.py Introduces TEST_METRICS, --test_metrics arg, warning helper, and metric-aware discrepancy pass injection.
docs/source/how-to/cli/cli-fast-test.md Documents --test_metrics and provides an example for speedup-only runs.

Comment thread olive/cli/base.py
@xadupre xadupre marked this pull request as ready for review June 22, 2026 15:29
@xadupre

xadupre commented Jun 22, 2026

Copy link
Copy Markdown
Member Author

@copilot test/cli/test_base.py:360: in test_add_discrepancy_check_pass_default_enables_all_metrics
assert "timing_iterations" not in pass_config
E AssertionError: assert 'timing_iterations' not in {'type': 'OnnxDiscrepancyCheck', 'reference_model_path': 'ref_model', 'report_output_dir': 'out_dir', 'max_mae': 0.1, ...}

Copilot AI commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

@copilot test/cli/test_base.py:360: in test_add_discrepancy_check_pass_default_enables_all_metrics
assert "timing_iterations" not in pass_config
E AssertionError: assert 'timing_iterations' no...

Fixed in the latest commit. The test was asserting the old behavior (default = all metrics, so timing_iterations absent). After the default changed to {"mae"} only, timing_iterations is set to 0. Updated the test name to test_add_discrepancy_check_pass_default_enables_mae_only and changed the assertion to assert pass_config["timing_iterations"] == 0.

@xadupre

xadupre commented Jun 22, 2026

Copy link
Copy Markdown
Member Author

@copilot fix test/cli/test_cli.py:178: in test_workflow_run_command_with_test_override
mock_run.assert_called_once_with(
/opt/hostedtoolcache/Python/3.12.13/x64/lib/python3.12/unittest/mock.py:961: in assert_called_once_with
return self.assert_called_with(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/opt/hostedtoolcache/Python/3.12.13/x64/lib/python3.12/unittest/mock.py:949: in assert_called_with
raise AssertionError(_error_message()) from cause
E AssertionError: expected call not found.
E Expected: run({'input_model': {'type': 'HfModel', 'model_path': 'hf-internal-testing/tiny-random-LlamaForCausalLM', 'load_kwargs': {'attn_implementation': 'eager', 'trust_remote_code': False}, 'test_model_config': {'hidden_layers': 2}, 'test_model_path': '/mnt/vss/_work/1/.pytest_basetemp/test_workflow_run_command_with1/output/test_model'}, 'output_dir': '/mnt/vss/_work/1/.pytest_basetemp/test_workflow_run_command_with1/output', 'passes': {'discrepancy_check': {'type': 'OnnxDiscrepancyCheck', 'reference_model_path': '/mnt/vss/_work/1/.pytest_basetemp/test_workflow_run_command_with1/output/test_model', 'max_mae': 0.1, 'report_output_dir': '/mnt/vss/_work/1/.pytest_basetemp/test_workflow_run_command_with1/output'}}}, list_required_packages=False, package_config=None, tempdir=None)
E Actual: run({'input_model': {'type': 'HfModel', 'model_path': 'hf-internal-testing/tiny-random-LlamaForCausalLM', 'load_kwargs': {'attn_implementation': 'eager', 'trust_remote_code': False}, 'test_model_config': {'hidden_layers': 2}, 'test_model_path': '/mnt/vss/_work/1/.pytest_basetemp/test_workflow_run_command_with1/output/test_model'}, 'output_dir': '/mnt/vss/_work/1/.pytest_basetemp/test_workflow_run_command_with1/output', 'passes': {'discrepancy_check': {'type': 'OnnxDiscrepancyCheck', 'reference_model_path': '/mnt/vss/_work/1/.pytest_basetemp/test_workflow_run_command_with1/output/test_model', 'report_output_dir': '/mnt/vss/_work/1/.pytest_basetemp/test_workflow_run_command_with1/output', 'max_mae': 0.1, 'timing_iterations': 0}}}, list_required_packages=False, tempdir=None, package_config=None)

Copilot AI commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

@copilot fix test/cli/test_cli.py:178: in test_workflow_run_command_with_test_override
mock_run.assert_called_once_with(
/opt/hostedtoolcache/Python/3.12.13/x64/lib/python3.12/unittest/mock.py:9...

Fixed in the latest commit. Added "timing_iterations": 0 to the expected discrepancy_check pass config in test_workflow_run_command_with_test_override, matching the new default mae-only behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants