Conversation
There was a problem hiding this comment.
Pull request overview
Updates Campaign.allow_* flags to use an AutoBool mechanism so callers always observe plain bool values at query time, while still supporting an “auto” mode.
Changes:
- Replaced
bool | UnspecifiedTypeallow-flag storage with privateAutoBoolattrs fields plus public bool-returning properties. - Updated validation to raise
IncompatibilityErrorfor incompatible allow-flag settings when continuous subspaces are involved. - Refreshed tests and changelog to cover/describe the new behavior (including
"auto").
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/test_campaign.py | Updates parametrization and assertions to validate lazy resolution of allow flags and new error type. |
| baybe/campaign.py | Introduces AutoBool-backed private fields, new validator, and public bool properties for allow flags. |
| CHANGELOG.md | Documents the behavioral change and a cache-related fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if space_type is SearchSpaceType.DISCRETE: | ||
| parameter = NumericalDiscreteParameter("p", [0, 1]) | ||
| else: | ||
| parameter = NumericalContinuousParameter("p", [0, 1]) |
There was a problem hiding this comment.
The test parametrizes space_type with SearchSpaceType.HYBRID, but the constructed parameter only yields either a discrete or continuous search space. As a result, the HYBRID case is not actually exercised (and Campaign.searchspace.type likely won’t be HYBRID). Consider constructing an actual hybrid search space for the HYBRID branch (e.g., by explicitly building a SearchSpace with both discrete and continuous subspaces) so the new HYBRID expectations are truly validated.
| _allow_recommending_already_measured: AutoBool = field( | ||
| alias="allow_recommending_already_measured", | ||
| default=AutoBool.AUTO, | ||
| converter=AutoBool.from_unstructured, | ||
| validator=_validate_allow_flag, | ||
| on_setattr=_set_with_cache_cleared, | ||
| kw_only=True, | ||
| ) | ||
| """Allow to recommend experiments that were already measured earlier. | ||
| Can only be set for discrete search spaces.""" | ||
| """Allow recommending experiments that have already been measured.""" | ||
|
|
||
| allow_recommending_already_recommended: bool | UnspecifiedType = field( | ||
| default=Factory( | ||
| _make_allow_flag_default_factory(default=False), takes_self=True | ||
| ), | ||
| _allow_recommending_already_recommended: AutoBool = field( | ||
| alias="allow_recommending_already_recommended", | ||
| default=AutoBool.AUTO, | ||
| converter=AutoBool.from_unstructured, | ||
| validator=_validate_allow_flag, | ||
| on_setattr=_set_with_cache_cleared, | ||
| kw_only=True, | ||
| ) | ||
| """Allow to recommend experiments that were already recommended earlier. | ||
| Can only be set for discrete search spaces.""" | ||
| """Allow recommending experiments that have already been recommended.""" | ||
|
|
||
| allow_recommending_pending_experiments: bool | UnspecifiedType = field( | ||
| default=Factory( | ||
| _make_allow_flag_default_factory(default=False), takes_self=True | ||
| ), | ||
| _allow_recommending_pending_experiments: AutoBool = field( | ||
| alias="allow_recommending_pending_experiments", | ||
| default=AutoBool.AUTO, | ||
| converter=AutoBool.from_unstructured, | ||
| validator=_validate_allow_flag, | ||
| on_setattr=_set_with_cache_cleared, | ||
| kw_only=True, | ||
| ) |
There was a problem hiding this comment.
These attrs fields were renamed to private _allow_* names with public-facing alias=.... Any existing code that introspects attrs via fields(Campaign).allow_recommending_* will now break (those fields no longer exist as attrs attributes). Update such call sites to reference the new attrs fields (e.g., fields(Campaign)._allow_recommending_already_measured) and use .alias when you need the user-facing name for messages/config output.
| return bool(self._allow_recommending_already_measured) | ||
|
|
||
| @property | ||
| def allow_recommending_already_recommended(self) -> bool: | ||
| """Allow recommending experiments that have already been recommended.""" | ||
| if self._allow_recommending_already_recommended is AutoBool.AUTO: | ||
| return False | ||
| return bool(self._allow_recommending_already_recommended) | ||
|
|
||
| @property | ||
| def allow_recommending_pending_experiments(self) -> bool: | ||
| """Allow recommending pending experiments.""" | ||
| if self._allow_recommending_pending_experiments is AutoBool.AUTO: | ||
| return False | ||
| return bool(self._allow_recommending_pending_experiments) |
There was a problem hiding this comment.
These properties rely on bool(AutoBoolValue) for non-AUTO cases. That’s easy to misread and can be brittle depending on how AutoBool is implemented (e.g., Enum truthiness vs IntEnum vs custom __bool__). Consider making the mapping explicit (e.g., comparing to AutoBool.TRUE/AutoBool.FALSE or using a dedicated AutoBool.to_bool() helper) to keep semantics unambiguous.
| return bool(self._allow_recommending_already_measured) | |
| @property | |
| def allow_recommending_already_recommended(self) -> bool: | |
| """Allow recommending experiments that have already been recommended.""" | |
| if self._allow_recommending_already_recommended is AutoBool.AUTO: | |
| return False | |
| return bool(self._allow_recommending_already_recommended) | |
| @property | |
| def allow_recommending_pending_experiments(self) -> bool: | |
| """Allow recommending pending experiments.""" | |
| if self._allow_recommending_pending_experiments is AutoBool.AUTO: | |
| return False | |
| return bool(self._allow_recommending_pending_experiments) | |
| return self._allow_recommending_already_measured is AutoBool.TRUE | |
| @property | |
| def allow_recommending_already_recommended(self) -> bool: | |
| """Allow recommending experiments that have already been recommended.""" | |
| if self._allow_recommending_already_recommended is AutoBool.AUTO: | |
| return False | |
| return self._allow_recommending_already_recommended is AutoBool.TRUE | |
| @property | |
| def allow_recommending_pending_experiments(self) -> bool: | |
| """Allow recommending pending experiments.""" | |
| if self._allow_recommending_pending_experiments is AutoBool.AUTO: | |
| return False | |
| return self._allow_recommending_pending_experiments is AutoBool.TRUE |
|
|
||
| ### Changed | ||
| - The `Campaign.allow_*` flag mechanism is now based on `AutoBool` logic, providing | ||
| well-defined Boolean values at query time while exposing the `AUTO` option to the user |
There was a problem hiding this comment.
The changelog mentions an AUTO option, but the user-facing input in tests/messages appears to be the string 'auto' (and/or AutoBool.AUTO). Consider clarifying the exact supported user inputs (e.g., true/false/'auto' in JSON/configs, and/or AutoBool.AUTO in Python) so external users know what to pass.
| well-defined Boolean values at query time while exposing the `AUTO` option to the user | |
| well-defined Boolean values at query time while exposing an automatic mode to the user | |
| (e.g., via the string `"auto"` in JSON/configs or `AutoBool.AUTO` in Python code) |
|
@AdrianSosic: The |
AVHopp
left a comment
There was a problem hiding this comment.
Except for the failing tests this looks good to me.
There was a problem hiding this comment.
is there anything in the doc that will make it easy to understand what the (finally resolved bollean) values are for the respective cases? Currently I think users would ahve to bother witht he logi. or execute code to underssand that,
Can we add a table? Or can the docstrings of the flags be expanded?
There was a problem hiding this comment.
You mean something like a table that shows how the auto-values resolve for the different flags and search space types? I have nothing against that, but do you see any way how we can ensure that stuff stays in sync (especially given that meddling with the flags sort of has been our yearly fun exercise over and over again)?
There was a problem hiding this comment.
well then lets have a static table, it wont change frequently anyway
Fixes #733.
The drawback of the previous approach was that the flag attributes had a union
bool | UnspecifiedTypetype, which meant that they could not be safely accessed without additional conversion logic, resulting in the bug reported in #733.To fix this, the user-facing value of the flags should always be a simple
boolat query time, while still offering a clever "auto-determine" mechanism that does not require the user to explicitly set them depending on the search space type.There are two options to achieve this:
AutoBoolapproach, where the user-specified value is stored as a private attribute and the public value is resolved lazily at query time.