refactor(budget-optimizer): collapse default_constraints into constraints#2570
Open
anevolbap wants to merge 6 commits into
Open
refactor(budget-optimizer): collapse default_constraints into constraints#2570anevolbap wants to merge 6 commits into
anevolbap wants to merge 6 commits into
Conversation
…ints Drop default_constraints and rename custom_constraints to constraints on BudgetOptimizer. Empty constraints auto-adds the default sum constraint; non-empty means the caller is in charge. Pass build_default_sum_constraint() explicitly to keep the default alongside customs. Old kwargs stay as deprecated aliases that warn and preserve current behavior for one release. On MMM.optimize_budget, default_constraints is now bool | None and only triggers the legacy path when explicitly set. Closes pymc-labs#1416.
anevolbap
force-pushed
the
refactor/1416-simplify-constraints
branch
from
93540f3 to
58f539c
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2570 +/- ##
==========================================
+ Coverage 93.89% 93.91% +0.01%
==========================================
Files 92 92
Lines 14062 14091 +29
==========================================
+ Hits 13204 13234 +30
+ Misses 858 857 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
… path Add tests for the two uncovered branches of the legacy translation in BudgetOptimizer._migrate_legacy_constraint_kwargs and for the legacy forwarding path in MMM.optimize_budget. Also pin the deprecation/removal versions in the warning message and docstrings to match the project convention (deprecated in 0.20.0, removed in 0.21.0).
…, deprecate set_constraints default kwarg The "Using default equality constraint" UserWarning fired on the documented happy path (empty constraints auto-add the default sum), turning it into noise. Drop it and clean up the 11 test assertion sites. The new contract is in the docstring instead. Also: deprecate the `default` kwarg of `set_constraints`, since it is now redundant under the new contract. Preserve the legacy strict behaviour for `default_constraints=False` with empty custom_constraints by raising a ValueError in the migration shim instead of silently auto-adding the default. Add tests for: the deprecated `set_constraints(default=...)` kwarg, the strict-no-constraint edge case, and Constraint round-trip through the legacy migrator.
…pe in docstring Add an inline comment in the migration validator explaining why the conflict path raises TypeError (re-raised as-is by Pydantic) while the edge case raises ValueError (wrapped into ValidationError, which is itself a ValueError so callers are unaffected). Also fix the docstring type for `default_constraints` on MMM.optimize_budget from `bool, optional` to `bool or None, optional` to match the actual annotation.
anevolbap
commented
May 15, 2026
Contributor
Author
anevolbap
left a comment
anevolbap
left a comment
There was a problem hiding this comment.
Adding a few inline notes to ease review.
| _constraints: dict = PrivateAttr() | ||
| _compiled_constraints: list[dict] = PrivateAttr() | ||
|
|
||
| @model_validator(mode="before") |
Contributor
Author
There was a problem hiding this comment.
Pydantic v2 idiom for renaming a Pydantic field at construction time. Field(deprecated=...) only fires on attribute access, not when kwargs are passed.
| # value error. | ||
| raise TypeError( | ||
| "Pass either `constraints` or the deprecated " | ||
| "`custom_constraints`/`default_constraints`, not both." |
Contributor
Author
There was a problem hiding this comment.
The last branch (empty custom_constraints with default_constraints=False) preserves the pre-PR error instead of silently auto-adding the default.
| ) | ||
| self.set_constraints(constraints=self.constraints) | ||
|
|
||
| def set_constraints(self, constraints, default=None) -> None: |
Contributor
Author
There was a problem hiding this comment.
default is reachable only from internal callers after this PR. Deprecating it now keeps the removal in the same cycle.
| model=self, | ||
| compile_kwargs=self.compile_kwargs, | ||
| ) | ||
| if default_constraints is None: |
Contributor
Author
There was a problem hiding this comment.
Two branches so the DeprecationWarning has one source of truth (BudgetOptimizer's validator).
anevolbap
changed the title
Collaborator
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Closes #1416. See the issue thread for design discussion. Notebook updates land in a follow-up PR.
Summary
custom_constraintstoconstraintsonBudgetOptimizer.default_constraintsfield. Emptyconstraintsauto-adds the default sum constraint; non-empty means the caller is in charge. Passbuild_default_sum_constraint()explicitly to keep the default alongside customs.MMM.optimize_budget:default_constraints: bool | None = None. Legacy path only runs when explicitly passed.UserWarning("Using default equality constraint"). It fired on the documented happy path under the new contract.defaultkwarg ofset_constraints(in-scope cleanup).default_constraints=Falsewith emptycustom_constraintsstill raises.Test plan
tests/mmm/test_budget_optimizer.py(29 pass, 8 new)tests/mmm/test_budget_optimizer_mmm.py+tests/mmm/test_cost_per_unit.py(78 pass, 1 new)