Deprecate SubspaceDiscrete.constraints#835
Conversation
…rete The constraints field on SubspaceDiscrete conflated two fundamentally different kinds of constraints: filtering constraints (eval_during_creation) that are applied during construction to reduce the candidate set, and batch constraints (eval_during_modeling) that operate at recommendation time to partition candidates into subsets. Storing filtering constraints after construction is redundant since exp_rep already reflects their effect, and batch constraints had no dedicated home, making them unreachable via from_dataframe entirely. This change separates the two concerns by introducing a dedicated batch_constraints field and deprecating the catch-all constraints field, with automatic migration of batch constraints for backward compatibility.
There was a problem hiding this comment.
Pull request overview
This PR refactors discrete-subspace constraint handling by separating creation-time filtering constraints from recommendation-time batch constraints, deprecating the legacy SubspaceDiscrete.constraints storage/usage, and removing ambiguous is_constrained convenience properties.
Changes:
- Introduces
SubspaceDiscrete.batch_constraintsand migrates legacyconstraints(argument/serialization) to batch constraints with deprecation warnings. - Updates discrete subspace constructors/serialization to split filtering vs batch constraints and discard filtering constraints after construction.
- Removes
is_constrainedfromSubspaceDiscrete,SubspaceContinuous, andSearchSpace, updating call sites and tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
baybe/searchspace/discrete.py |
Adds batch_constraints, migration/deprecation logic, constructor updates, and a temporary structure hook for legacy constraints key. |
baybe/searchspace/core.py |
Updates SearchSpace.constraints to use discrete batch_constraints; removes is_constrained. |
baybe/searchspace/continuous.py |
Removes is_constrained and updates sampling logic to check constraints directly. |
baybe/searchspace/_filtered.py |
Drops deprecated _constraints when cloning filtered subspaces. |
tests/test_deprecations.py |
Adds regression tests for legacy constraints argument/key migration into batch_constraints. |
CHANGELOG.md |
Documents additions/changes/deprecations around constraint API updates and is_constrained removal. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - `Campaign.n_fits_done` and `Campaign.n_batches_done` attributes | ||
| - `SubspaceDiscrete` ignores any `empty_encoding` when provided | ||
| - `SubspaceDiscrete` no longer accepts a `comp_rep` argument | ||
| - `SubspaceDiscrete.constraints` field |
| def __attrs_post_init__(self) -> None: | ||
| """Migrate deprecated ``constraints`` argument to ``batch_constraints``.""" | ||
| # >>>>>>>>>> Deprecation | ||
| if self._constraints is not None: |
There was a problem hiding this comment.
This silently drops all non-batch constraints a user provided. Should we at least give a warning or similar if they still provided them, just to make sure that users are aware of this?
There was a problem hiding this comment.
The thing is: there is really no change in terms of the underlying behavior! This is exactly what also happened before because – as explained in the PR description – the constraints were stored but nowhere actually used. So the runtime behavior is 100% identical. In the past, I guess this weird interface didn't bother anyone because probably 0% of users ever explicitly called the regular constructor. People would usually go via from_product, from_dataframe and from_simplex. So in that sense, I don't think we have to warn here because we're giving the users the same experience as before. Or what is your opinion?
There was a problem hiding this comment.
Well, doing it wrong and bad in the past is not an argument why not to do it better. If we now see that we should have actually warned people previously and agree on that (do we?) then we can still include this now.
| batch_constraints: Optional batch constraints to be applied at | ||
| recommendation time. |
There was a problem hiding this comment.
Technically, those are not optional as we do not allow None but provide an empty list as default. So we should either also accept None as input or change the docstring to (Potentially empty) list of batch constraints to be applied at recommendation time. or similar
There was a problem hiding this comment.
I think you are maybe confusing the term "optional" with Python's legacy typing.Optional. While the latter was indeed a shorthand for | None, optional in natural language simply means you can but do not need to provide it, right?
There was a problem hiding this comment.
Depends on the layer of optionality in my opinion: For the function, it is not optional - you have to provide a value. As a user, you do not need to do so since we provide a default - so probably we need to align on what Optional means on the semantic level. And I'd say that since it is not optional from the POV of the function, it should not be called optional, even if it might be from the user's perspective. Or what is your take here (or maybe there is even some well-defined semantics for this in the python world)?
myrazma
left a comment
myrazma
left a comment
There was a problem hiding this comment.
Looks good to me, just some minor comments.
I like the separation of filtered and batch constraints. Previously when they were combined, it created the false impression that the filtered constraints were applied to exp_rep after creating the Searchspace, which they are not. So this update removes that ambiguity and hopefully prevent user confusion.
@myrazma: yes, but the user will never actually see this state of the code. Once merged to |
Related to #794
The
constraintsattribute onSubspaceDiscreteconflated two fundamentally different roles:Storing both under a single field was misleading – filtering constraints are only relevant during construction, and even then only for certain construction routes: when building via
from_dataframe, the candidate grid is already fully specified by the caller, so filtering constraints were meaningless from the very beginning and had no effect whatsoever. Retaining them on the object after construction served no purpose and gave a false impression that they remained active throughout the object's lifetime. As a further consequence,from_dataframenever accepted any constraint objects at all – since filtering constraints are not needed there, and batch constraints were introduced later, users had no way to attach batch constraints when constructing a subspace from a dataframe.This PR makes the distinction explicit and removes the dead storage:
SubspaceDiscrete.batch_constraints: a dedicated field forDiscreteBatchConstraintinstances that are evaluated during recommendation.from_dataframenow accepts abatch_constraintsargument – fixing the previously missing ability to attach batch constraints when constructing from a dataframe. Filtering constraints passed tofrom_product/from_simplexare applied during construction as before and then discarded – there is nothing left for them to do.SubspaceDiscrete.constraints: passing it emits aDeprecationWarning; anyDiscreteBatchConstraintinstances are automatically migrated tobatch_constraintsto ease the transition.is_constrainedproperties fromSubspaceDiscrete,SubspaceContinuous, andSearchSpace: these were thin Boolean wrappers that obscured which kind of constraint was being checked and, in the discrete case, were backed by the now-meaningless stored filtering constraints.