Repository navigation
fix: keep suggested configs independent of the portfolio - #1632
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The focused fix preserves selection behavior and includes targeted regression coverage, with no unresolved findings.
No open issues 🎉
What changed in this PR
Prevents callers from modifying FLAML’s cached portfolio or caller-provided predictors through suggested configurations.
Changes:
- Deep-copies selected configurations before removing
FLAML_sample_size. - Documents that returned configurations can be safely modified.
- Adds regression coverage for bundled estimators and nested caller-provided values.
| File | Description |
|---|---|
test/default/test_defaults.py |
Adds tests for suggestion independence and predictor preservation. |
flaml/default/suggest.py |
Copies selected configurations and documents mutation safety. |
Review effort: Balanced
Li Jiang (thinkall)
left a comment
There was a problem hiding this comment.
Overall review of the complete PR: no blocking code issue found; approval withheld pending CI.
Deep-copying each selected portfolio entry before sample-size removal protects both cached and caller-supplied predictors, including nested mutable values, without changing selection order or the result structure. The focused regression command passed 9 tests locally. The review-code harness is clean and Copilot's current Balanced review recommends approval with no open issues. However, the current head has failing build checks, so it does not meet the required all-CI-passed approval gate. This is a formal comment review, not an approval.
Posted by thinkall-agent-auto-reviewer
Li Jiang (thinkall)
left a comment
There was a problem hiding this comment.
Overall review at 3b7f286e1a117bb3eba8fe34125ccfe386b8bb93: re-reviewed the complete PR with review-code, including the copy boundary, predictor cache, caller-owned nested values, and downstream configuration consumers. Copying each selected portfolio entry before cleanup prevents mutations from leaking into later suggestions while preserving selection order and the returned configuration shape. No remaining correctness issue found.
Validation: python -m pytest test\default\test_defaults.py -k 'suggested_config_changes or suggest_config_preserves' -q --disable-warnings passed (9 passed, 12 deselected). 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
Why are these changes needed?
suggest_config()returns dictionaries from the cached configuration portfolio. Updating a suggested hyperparameter therefore changes later suggestions in the same process. It also removesFLAML_sample_sizedirectly from a caller-provided predictor.Copy each selected configuration before removing the sample-size key. This keeps the existing selection and returned values while allowing callers to customize a suggestion without changing the predictor or subsequent calls.
For example, using the bundled random-forest regression portfolio:
The assertion fails before this change: the second call returns the caller's
1instead of the portfolio's960.Related issue number
No existing issue; the reproducer is above.
Checks
Local validation on Windows with Python 3.12.14, NumPy 1.26.4, pandas 3.0.6 and scikit-learn 1.9.1:
k=1prefix and emptyk=0result.pre-commit run --all-files: all 15 checks pass on both the pristine base and the candidate.flaml/defaultusing the same 13 selected tests; the covered fraction does not decrease. This is focused coverage, not a full-project result.