Fix autotuner crash when an experiment produces no metric - #8421
Fix autotuner crash when an experiment produces no metric#8421Rakshit-gen wants to merge 3 commits into
Conversation
get_best_space_record compared metric_val > best_space_record[1] unconditionally. When an experiment produces no metric (for example an OOM during autotuning), metric_val is None, and once the first record in a space is None the very next comparison raises TypeError: '>' not supported between instances of 'NoneType' and 'NoneType'. This crashes the whole tuning run and throws away every valid result gathered so far, in both the fast-mode and full-tuning call sites in tune_space, plus get_best_space_records. Records with a None metric are no longer eligible to become the best record. They still count toward the space's total experiment count, they just cannot win the comparison, which matches how a failed run should be treated everywhere the function is called from. Fixes deepspeedai#3475 Signed-off-by: rakshit-gen <sisodiarakshit456@gmail.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c2575122a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if metric_val is None: | ||
| # a run that did not produce a metric (e.g. OOM) is not a valid candidate | ||
| continue |
There was a problem hiding this comment.
When a fast micro-batch pass records only one None metric (for example, a user-specified micro-batch list whose run writes a null metric), this branch now makes get_best_space_record() return None; tune_space() then unconditionally evaluates fast_best_record[0]['name'] at line 586. That turns a case which previously completed with a single such record into a 'NoneType' object is not subscriptable failure, so the caller must avoid dereferencing the missing record.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, this was a real regression. Fixed in 7d770d0: fast_best_record[0]['name'] now goes through the same None-safe ternary as fast_best_mbs and fast_best_metric_val on the lines right above it.
| assert expected_num_gpus == tuner.exp_num_gpus | ||
|
|
||
|
|
||
| def test_get_best_space_record_ignores_runs_without_a_metric(tmpdir): |
There was a problem hiding this comment.
Commit 23e6d10f3f945c85542953b0531273181fdf8150 is a non-merge commit, but its message has no Signed-off-by: trailer, violating the repository's mandatory commit requirement; add the author sign-off before this change is merged.
AGENTS.md reference: AGENTS.md:L8-L8
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This looks like it's pointing at a commit that isn't part of this PR (23e6d10f3f9...). The actual commit here is 5c25751, which does have a Signed-off-by trailer, you can see it with git log or the commits tab.
tune_space logs fast_best_record[0]['name'] unconditionally right after computing fast_best_metric_val and fast_best_mbs with a None-safe ternary. Now that get_best_space_record can return None when every experiment in a space had no metric, that log line needs the same guard the other two already have. Signed-off-by: rakshit-gen <sisodiarakshit456@gmail.com>
|
cc @tjruwase for visibility as well |
2 participants
Fixes #3475
The bug
get_best_space_recordpicks the best record in a tuning space with:An experiment that produces no metric (an OOM during autotuning is the
common case) is still recorded, with
metric_valset toNone. Once aNone-valued record becomesbest_space_record, the next comparisondoes
metric_val > best_space_record[1], and if both sides happen tobe
None(or the incoming one is a real number compared against aNonebest) this raises:This isn't limited to
get_best_space_recorditself:tune_spacecalls it both in fast-tuning mode and after full tuning to pick the
overall best micro batch size, and
get_best_space_recordscalls itonce per tuning space to build the final results table. A single space
where every experiment OOM'd is enough to crash the whole autotuning
run and lose every valid result gathered from the spaces tuned before
it.
The fix
A record with a
Nonemetric is no longer eligible to become thebest record. It's still counted toward the space's total number of
experiments (
space_num_exps), it just can't win the comparison. Thismeans
get_best_space_recordnow returnsNoneonly when a space hasno experiments with a usable metric at all, which the existing callers
already handle correctly (they fall back to
0or-1in that case),so no other call site needed to change.
Testing
Added
test_get_best_space_record_ignores_runs_without_a_metrictotests/unit/autotuning/test_autotuning.py, covering:metric_val=None-> no crash, returnsNoneNoneand real metrics -> the real one wins regardless of position, and the experiment count still includes the failed runsConfirmed this test fails with the exact
TypeErrorabove on currentmaster and passes with the fix. Also ran the full existing
tests/unit/autotuning/test_autotuning.pysuite (12 tests) and itpasses. Ran
pre-commiton both changed files (yapf, flake8,check-torchdist, check-license, codespell) with no issues. This is a
pure CPU, no-GPU-needed unit test path, no integration/training-loop
behavior is touched.
cc @loadams (CODEOWNERS for
deepspeed/autotuning/)