Skip to content

fix(test): widen iter-time steady-state window for short tests #5023

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ko3n1g merged 1 commit into from
May 27, 2026
+2 −3
Merged

fix(test): widen iter-time steady-state window for short tests #5023

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions tests/functional_tests/python_test_utils/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,9 +208,8 @@ def pipeline(
]

if metric_name == "iteration-time":
# Restrict iter-time aggregation to the steady-state window
# (steps 30-45) so the warmup step does not dominate the median.
steady_window = range(30, 46)
max_golden_step = max(golden_value.values.keys()) if golden_value.values else 0

@wujingyue wujingyue May 27, 2026

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.

Suggested change
max_golden_step = max(golden_value.values.keys()) if golden_value.values else 0
max_golden_step = max(golden_value.values.keys(), default=0)

steady_window = range(5, 21) if max_golden_step <= 25 else range(30, 46)

@wujingyue wujingyue May 27, 2026

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.

Sounds a bit arbitrary. Conceptually, if warmup is still ongoing after iteration 5, steady_windows isn't steady. If not, we should just set the range to range(5, 21) instead of an if-then-else. Wdyt?

@ko3n1g ko3n1g May 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This PR is a stop gap to another issue, so I’ll need to merge it. But open to discuss another follow-up.

Usually our tests run for 50 steps, so we take 30:46 for averaging.
however some tests only for 25 steps, so the best I could come up with was to choose 5:21.

Not sure how to de-select noise steps better for those tests..

@wujingyue wujingyue May 27, 2026

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.

This PR is a stop gap to another issue, so I’ll need to merge it

Sure.

Not sure how to de-select noise steps better for those tests..

In that case, the warmup threshold should probably be set by individual tests rather than being speculated by the test infra here.

actual_value_list = [
value
for value_step, value in actual_values[metric_name].values.items()
Expand Down
Loading