镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

fix: keep processing the search space after a list low-cost point - #1637

Merged
Li Jiang (thinkall) merged 4 commits into
microsoft:mainfrom
anishmehta24:fix/add-cost-to-space-early-return
Oct 19, 2026
Merged

Li Jiang (thinkall) merged 4 commits into
microsoft:mainfrom
anishmehta24:fix/add-cost-to-space-early-return

Conversation

@anishmehta24

Copy link
Copy Markdown
Contributor

Why are these changes needed?

add_cost_to_space loops over the keys of the search space, but when a categorical has a list low-cost point (one low-cost config per choice) it hits a return inside the loop. Every key after it is skipped: no low_cost_point, no bounded, and nested choices never get their const. The function also returns None instead of the constants.

With a second nested choice after the first one, tune.run crashes:

space = {
    "c": tune.choice([{"a": tune.randint(1, 10)}, {"b": tune.uniform(0, 1)}]),
    "x": tune.randint(1, 100),
    "y": tune.choice([{"p": tune.uniform(0, 1)}, {"q": tune.uniform(0, 1)}]),
}
tune.run(lambda c: {"metric": c["x"]}, config=space,
         low_cost_partial_config={"c": [{"a": 1}, {"b": 0}], "x": 1},
         metric="metric", mode="min", num_samples=10)
# AttributeError: 'Categorical' object has no attribute 'const'

Changing the return to continue keeps what happens for that key and lets the loop finish.

Related issue number

None found.

Checks

test_add_cost_to_space_keys_after_nested_choice fails on main and passes with the change.

add_cost_to_space returned from inside the loop when a categorical
had a list low-cost point, so the keys after it never got their
low_cost_point, bounded flag or categorical const, and the function
returned None instead of the constants. A later nested choice then
crashed BlendSearch with AttributeError: 'Categorical' object has no
attribute 'const'.

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.

🟢 Approval recommended

The localized control-flow fix addresses the reported failure and includes focused regression coverage.

No open issues 🎉

What changed in this PR

Fixes premature termination in add_cost_to_space so search-space initialization continues after a categorical list low-cost point.

Changes:

  • Replaces return with continue to process remaining keys and return constants.
  • Adds regression coverage for initialization and successful tune.run execution.
File Description
test/​tune/​test_space.py Tests subsequent keys, returned constants, and tuning execution.
flaml/​tune/​space.py Continues initialization after handling a list low-cost point.

Review effort: Balanced

@thinkall Li Jiang (thinkall) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Overall review of the complete PR: no blocking code issue found; approval withheld pending CI.

Replacing the early return with continue preserves list-valued categorical handling while allowing later domains to be initialized and the constant dictionary to be returned. The focused test module passed all 3 tests locally, including the added tuning regression. The review-code harness is clean and Copilot's current Balanced review recommends approval with no open issues. Build checks on the current head are failing, so it does not satisfy the all-CI-passed approval gate. This is a formal comment review, not an approval.

Posted by thinkall-agent-auto-reviewer

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.

🟢 Approval recommended

The narrow control-flow fix addresses the reported failure with targeted regression coverage and no unresolved findings.

No open issues 🎉

Review effort: Balanced

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.

🟢 Approval recommended

The localized control-flow fix has targeted regression coverage and no unresolved findings.

No open issues 🎉

Review effort: Balanced

@thinkall Li Jiang (thinkall) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Overall review at 7184aca9c957e58fe2176a51579f1e454cbaa122: re-reviewed the complete PR with review-code, including recursive categorical initialization and downstream consumers. Continuing the outer loop after handling a list low-cost point preserves that category's handling while allowing later constants, bounds, and nested choices to be initialized and the constants dictionary to be returned. No remaining correctness issue found.

Validation: python -m pytest test\tune\test_space.py -q --disable-warnings passed (3 passed). The new regression checks suffix constants, low-cost metadata, boundedness, subsequent nested-category initialization, and ten completed tuning trials. The whole-PR Low harness was clean, and its provenance was audited and committed locally. All discussion threads are resolved, current-head Balanced Copilot explicitly recommends approval, check contexts pass, and the actual Build, docs, and Code formatting workflow runs completed successfully on this head.

Posted by thinkall-agent-auto-reviewer

@thinkall
Li Jiang (thinkall) added this pull request to the merge queue Oct 19, 2026
Merged via the queue into microsoft:main with commit eafc621 Oct 19, 2026
16 checks passed
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