Add support for custom intervals #814
Merged
Conversation
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
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
model_analyzer/config/generate/brute_plus_binary_search_run_config_generator.py
Fixed
Show fixed
Hide fixed
model_analyzer/config/generate/quick_plus_concurrency_sweep_run_config_generator.py
Fixed
Show fixed
Hide fixed
nv-braf
reviewed
Jan 26, 2024
model_analyzer/config/generate/perf_analyzer_config_generator.py
Outdated
Show resolved
Hide resolved
model_analyzer/config/generate/perf_analyzer_config_generator.py
Outdated
Show resolved
Hide resolved
model_analyzer/config/generate/quick_plus_concurrency_sweep_run_config_generator.py
Outdated
Show resolved
Hide resolved
tgerdesnv
force-pushed
the
tgerdes-fix-custom-intervals
branch
from
1b24043 to
38abbe8
Compare
tgerdesnv
force-pushed
the
tgerdes-fix-custom-intervals
branch
from
fae1195 to
6b3a199
Compare
tgerdesnv
commented
Mar 30, 2024
the-david-oy
force-pushed
the
tgerdes-fix-custom-intervals
branch
5 times, most recently
from
fff4ec5 to
c038b73
Compare
the-david-oy
force-pushed
the
tgerdes-fix-custom-intervals
branch
2 times, most recently
from
aa265ad to
543bfeb
Compare
Improve logging: don't log load mode when using request-intervals
When using request-intervals, the load pattern is user-specified
and fixed (not being varied by Model Analyzer), so we shouldn't
log it. This follows MA's principle of only logging parameters
that MA is actively sweeping/varying.
Output:
- Before: 'Profiling model_config_11: request-intervals=./intervals.txt'
- After: 'Profiling model_config_11' (or with batch size if > 1)
Fix test expectations after changing batch-size default to 1
Updated test_perf_analyzer.py to expect -f test_model-results.csv
and -b 1 in CLI strings, matching the changes from:
- Setting default batch-size to 1 in PerfAnalyzerConfig.__init__
- Restoring latency-report-file in setUp()
Three tests were failing because expected strings didn't include
these parameters:
- test_perf_analyzer_boolean_args
- test_perf_analyzer_additive_args
- test_perf_analyzer_ssl_args
Fix tests
Remove broken test_run and fix test_get_cmd_single_model
Changes:
1. Removed test_run method (lines 240-535)
- Doesn't exist on main
- Integration test requiring actual Triton/perf_analyzer binaries
- Has incomplete mocking, never passed
- Not related to request-intervals feature
2. Fixed test_get_cmd_single_model expected command
- Added -f test_model-results.csv to expected output
- This matches the actual output after we restored
latency-report-file in setUp()
Fix test_run by adding missing _verify_output_files_exist mock
The test_run method was failing because it was trying to actually
execute perf_analyzer and verify files exist. Added the missing
patch.object() mock for _verify_output_files_exist that returns True,
which was present in the original test_pa_csv_output but missing when
it was refactored into test_run.
Applied the mock to all 16 patch blocks in the test that call
perf_analyzer.run().
All 13 tests in test_perf_analyzer.py now pass.
Restore test_pa_llm_csv_output from main to avoid regression
The test_pa_llm_csv_output test exists on main and tests LLM metrics
parsing functionality. It was accidentally removed in the original PR's
"fix unit tests" commit (b21f41d) when the author consolidated tests
into test_run.
Since this is a feature branch, we must not introduce regressions from
main. Restored the test from main (lines 502-703) along with all 19
required LLM metric imports.
All 14 tests now pass.
Clean up comments
Replace test_run with test_pa_csv_output from main
The test_run test was created in the original PR to replace
test_pa_csv_output, but since main has test_pa_csv_output (not
test_run), we need to use main's version to avoid divergence.
Both test_run and test_pa_csv_output test PA CSV parsing, but
test_pa_csv_output is the canonical version on main. Replaced
test_run (lines 259-588) with test_pa_csv_output from main
(lines 271-501).
Our branch now matches main's test structure:
- test_pa_csv_output (general PA CSV parsing)
- test_pa_llm_csv_output (LLM-specific CSV parsing)
All 14 tests pass.
Add unit tests for request-intervals feature
Added 4 unit tests to verify the request-intervals feature works correctly:
1. test_request_intervals_in_inference_load_args
- Verifies request-intervals is in the inference_load_args list
- Confirms all 3 load args (concurrency, request-rate, request-intervals) are present
2. test_request_intervals_preserved_in_config
- Tests that request-intervals value is preserved when set in PerfAnalyzerConfig
3. test_request_intervals_in_cli_string
- Verifies request-intervals appears correctly in the CLI string
- Format: --request-intervals=./intervals.txt
4. test_request_intervals_mutually_exclusive
- Tests that when request-intervals is set, other load parameters
(concurrency-range, request-rate-range) do NOT appear in CLI string
- This is the core feature: preventing MA from overriding user's request-intervals
All 18 tests pass (14 existing + 4 new).
the-david-oy
force-pushed
the
tgerdes-fix-custom-intervals
branch
from
543bfeb to
7cc4bf0
Compare
debermudez
reviewed
Dec 2, 2025
Contributor
debermudez
left a comment
debermudez
left a comment
There was a problem hiding this comment.
Couple questions. Nice job getting this cleaned up.
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.
Allow Model Analyzer to support --request-intervals, as Perf Analyzer does. Custom intervals work locally, and the user error is gone (confirmed via no PA error and the fact that the PA command includes --request-intervals file).
Fixes: #808
Screenshot of MA running successfully after changes:
Screenshot with verbose logging:
Command after changes:
model-analyzer profile -f config.yamlconfig.yaml:
intervals.txt: