Move `allow_*` flags to `Campaign` #423

AdrianSosic · 2024-11-09T19:46:32Z

This PR builds upon #412 and finalizes the metadata migration from search spaces / recommenders to Campaign, making Campaign the only stateful class:

All allow_* flags are now passed directly to Campaign, strengthening the role of Campaign as the class for metadata handling / tracking the progress of an experimentation path.
A corresponding deprecation mechanism is set into place.
The documentation gets updated.
allow_repeated_recommendations is renamed to allow_recommending_already_recommended because i) the original name was imprecise in that it also suggested that repetitions are disallowed within a batch and ii) the new follows the same pattern as the other two flags.
The flags are now context-aware, i.e. setting flags in contexts where they have no effect now raises an error. This avoids surprises on the user side, which have been reported multiple times.
Replaces the private class AnnotatedSubspaceDiscrete with a new private class FilteredSubspaceDiscrete taking over the original role but without the necessity of being metadata aware.

Coming next

The changes introduced here and in #423 bring significant improvements from a user interface perspective in that:

Metadata handling now happens entirely behind the scenes
No more fiddling with internal low-level objects (like setting Boolean values in metadata dataframes) is necessary to control candidate generation is necessary. Instead, there is now an interface that enables control via user-level objects such as Constraint objects and candidates dataframes.
The pandas part of the current discrete search space implementation is less entangled in the code base

This paves the way for upcoming enhancements:

A SubspaceDiscreteProtocol is already in sight, which allows seamless integration of other backends (polars, databases, etc) and will help us complete the ongoing polars transition.
An improved user interface for manipulating existing state spaces a la SubspaceDiscrete.filter(constraints), which can also become the backbone of the current constraints logic

AdrianSosic · 2024-11-21T09:01:15Z

baybe/recommenders/pure/nonpredictive/base.py

-                f"purely discrete spaces and "
-                f"{fields(self.__class__).allow_recommending_pending_experiments.name}"
-                f"=False.",
+                f"'{self.__class__.__name__}' does not use this information.",


@Scienfitz: I remember us having a longer discussion about this part in one of the previous PRs. With the new changes, I think the semantics are now considerable easier but we probably still need to iterate once again to align what the desired behavior should be

wait now the warning is printed whenever pending experiments is provided independent of the flag, that can be annoying

eg what happens if I pass pending experiments because I want to exclude them (flag on True), I'd always get this warning now or not?

Your conclusion is correct but I think your expectation is wrong. That's the point I tried to convey in today's meeting but didn't succeed :D Let's go trough the cases:

Recommender Level

Here, the flags don't exist, and we need to disentangle the two concerns of

informing the algorithm about pending points. This is done by passing pending_experiments to recommend

Excluding pending points from the candidate set. This is the filter part that is still missing. As I said, controlling what is recommendable on the search space level now always goes via filtering, i.e. creating a new object, since there is no more internal state we could control. So in the future, we can flexibly control this (in general, not just for pending points!) via recommender.recommend(searchspace=searchspace.filter(...))

If you consider the above, then the conclusion is that RandomRecommender(pending_experiments=df) should always raise the error, because the algorithm can't handle the info and excluding experiments would go via the filtering route instead.

Campaign Level

Same separation of concerns:

Informing about pending points is done in the same way, by passing pending_experiments to recommend.

Excluding pending points.

The second part is probably where the misunderstanding is: Excluding the pending points by passing pending_experiments + setting the flag to False is not just excluding the pending points but also informing the algorithm, so you'd be mixing both concerns. And in the non-predictive recommender context, one of the two concerns doesn't apply / make sense. So the warning would be justified. If you really only wanted to exclude, you'd go via toggling, which would be fine. But really the scenario is a bit exotic anyway because if you have pending points, you wouldn't want to use a nonpredictive recommender in the first place. And this is exactly what the warning tells you!

Does that make sense?

If I get your points here right, I'd also favor the variant where warnings are always being printed. If people are bothered by this, people can still globally deactivate warnings.
In general, I think that we should als re-visit in general how we handle warnings as I think there are quite some places where people might want to deactivate one type of warning while keeping another one. But this should not be done here :D

I dont think the words pending experimetns were mentioned once in the meeting, so not sure what you refer to...

Also, the argument that something is only possible if the imagined future with a filter method need to be discarded. We already filter now, just call it toggling, there is no need to defer anything into that imagined future because of it

If we look at the defacto state of the code now, non-pred recommenders can be passed pending experiments and could create this perma warning - This is not good, I remind you of the situation where the simulation module raised thousands of warnings because of the intial data. While its true that it can simply be discarded, users are confused and wonder rightfully whether they did something wrong, but in reality it was just us overly eager raising warnings.

So it seems simply to me the cleanest solution is pending experiments arg needs to be removed for non-pred recommenders (in the raising error sense to not meddle witht he protocol). The warning in recommenders becomes obsolete then. The campaign needs to decide whether it passes pending experiments or uses toggling or raises a warning if the respective combo appears - this shifting of responsibilities is akin to what you did with the other flag warnings - and again I dont see how that should be affected by further changes to renaming/moving our toggling/fitlering in an imaginary future, these imaginations relate to seachspace changes, my args are purely based on cmapaign-recommender topics.

so we do not trade error for warnings

Yes, we would. Because your argument was:

non-pred recommenders can be passed pending experiments and could create this perma warning - This is not good

So I can literally just do RandomRecommender.recommend(batch_size=..., searchspace=..., pending_experiments=...) as a single call without any context and would get 1) a warning in my version 2) an error in your version.

While changing the protocol is something to consider, I think it's out-of-scope for this PR. Hence, let's think about a clean intermediate solution that does not break later.

Can you give me a concrete suggestion in terms of the three options you mentioned above and then we see if we can get it running in a reasonable way?

I dont want to suggest a verison where an error is triggered
this is achieved by the campaign taking care of providing pending_experiments or not, something that doesnt seem to appear in the suggestions above

I think that should be independent of the protocol change. Is the protocol actually a big deal? is it ok if a class implementing the protocol "adds" an argument to the method? then is should be rather trivial

does it make sense to you?

let non-predictive recommenders throw an error if provided pending expeirments

make campaign take care that no such error is actually thrown if recommend is called via campaign. the campaign can decide its warning/error logic based on the old flags just as the logic was inside recommenders before

the protocol change is not necessary for that

Do you need an additional opinion here? This discussion goes on for quite some time already. Or do we want to discuss this in our Meeting this week?

Let's align once and for all in the dev meeting 🙃

Fixes #371 by making `SubspaceDiscrete` stateless. ### Current Approach * The `SubspaceDiscrete.metadata` attribute gets deprecated and the responsibility of metadata handling is shifted to `Campaign` * The new mechanism is not yet final (see out of scope below) but designed in a way that allows to implement upcoming changes in a non-breaking manner. In particular: * The metadata handling mechanism is redesigned in that the actual metadata representation is completely hidden from the user, i.e. campaign manages the data in form of private attributes. This is to avoid further lock-in into `pandas` as our search space backend and prepares for future search space improvements by abstracting away the specific implementation details, enabling us to easily offer other backends (polars, databases, etc) in the future. * The `allow_*` flags are not yet migrated to the `Campaign` class, but the `AnnotatedSubspaceDiscrete` allows to migrate them in a follow-up PR (#423) without causing much friction * A new user-facing method `Campaign.toggle_discrete_candidates` now allows convenient and dynamic control over the discrete candidate set, avoiding any fiddling with the backend dataframes and index manipulations. The positive effect can be seen in the much cleaner code parts of the simulation package. ### Out of scope / (potentially) coming next * Migration of `allow_*` flags in order to make `Campaign` the unique place where the concept of metadata exists, i.e. campaigns will be the only stateful objects. A PR taking care of this should follow soon because the `get_candidates` signature of `SubspaceDiscrete` currently makes not much sense, as it expects these flags in a context where metadata does not exist. * Once the flags are migrated, the `AnnotatedSubspaceDiscrete` might become obsolete since the `Campaign` class can then theoretically filter down the space before passing it to the recommender. This however requires an efficient implementation that does not cause unnecessary dataframe copies. * Actually turning the state space classes `frozen`. There a few other things that should be addressed at the same time (i.e. general cleanup of the classes).

baybe/utils/basic.py

AVHopp

Very brief set of initial comments. More to come.

CHANGELOG.md

baybe/utils/basic.py

AVHopp

LGTM, just some minor questions/comments

AVHopp · 2024-11-26T15:03:01Z

baybe/searchspace/_filtered.py

+        kw_only=True,
+        eq=cmp_using(eq=np.array_equal),
+    )
+    """The filtering mask. ``True`` denote elements to be kept."""


AVHopp · 2024-11-26T15:04:38Z

baybe/searchspace/_filtered.py

+class FilteredSubspaceDiscrete(SubspaceDiscrete):
+    """A filtered search space representing a reduced candidate set."""
+
+    mask: npt.NDArray[np.bool_] = field(


Can we maybe rename it to something like mask_for_keeping? Would make it a bit clearer in other contexts. Can however also understand if this is to cumbersome and you want to keep mask due to its simplicity. If that is the case, feel free to just resolve :)

in other places we also have mask_todrop etc so why not mask_tokeep/mask_keep or mask_todrop again, depending on the purpose

tests/conftest.py

tests/test_campaign.py

baybe/recommenders/pure/base.py

Scienfitz · 2024-11-26T09:44:51Z

baybe/recommenders/pure/nonpredictive/base.py

-                f"purely discrete spaces and "
-                f"{fields(self.__class__).allow_recommending_pending_experiments.name}"
-                f"=False.",
+                f"'{self.__class__.__name__}' does not use this information.",


wait now the warning is printed whenever pending experiments is provided independent of the flag, that can be annoying

eg what happens if I pass pending experiments because I want to exclude them (flag on True), I'd always get this warning now or not?

baybe/searchspace/_filtered.py

baybe/campaign.py

Scienfitz · 2024-11-26T09:55:31Z

docs/userguide/async.md

-only take pending points into consideration if the recommender flag
-[allow_recommending_pending_experiments](baybe.recommenders.pure.nonpredictive.base.NonPredictiveRecommender.allow_recommending_pending_experiments)
-is set to `False`. In that case, the candidate space is stripped of pending experiments
-that are exact matches with the search space, i.e. they will not even be considered.


the fact that pending exps can still be excluded for those recommenders via the allow flags of the campaign needs to be mentioned here (which is roughly the updated equivalent of what you've deleted)

Waiting for the outcome of this discussion first

tests/conftest.py

tests/test_deprecations.py

The original name allow_repeated_recommendations is not accurate since it also suggests that repeated configurations within a batch would be excluded, which is not the case.

Simply using `None` to represent an unspecified flag would be problematic since it evaluates to `False` in an if context, which is unintented. Instead, an unset flag occurring in such a context indicates a misconfiguration and should throw an error.

AVHopp

Since most if my wishes have been implemented, here you go with your approve :)

AdrianSosic added the refactor label Nov 9, 2024

AdrianSosic self-assigned this Nov 9, 2024

AdrianSosic mentioned this pull request Nov 9, 2024

Immutable searchspace #412

Merged

AdrianSosic force-pushed the refactor/allow_flags branch 3 times, most recently from 2ccb684 to f79b557 Compare November 14, 2024 07:14

AdrianSosic force-pushed the refactor/allow_flags branch from 0987d10 to 99d0428 Compare November 21, 2024 07:59

AdrianSosic added the enhancement Expand / change existing functionality label Nov 21, 2024

AdrianSosic force-pushed the refactor/allow_flags branch from 491c79d to 88ab485 Compare November 21, 2024 08:18

AdrianSosic marked this pull request as ready for review November 21, 2024 08:36

AdrianSosic requested review from Scienfitz and AVHopp as code owners November 21, 2024 08:36

AdrianSosic commented Nov 21, 2024

View reviewed changes

AdrianSosic force-pushed the refactor/allow_flags branch from 88ab485 to 818741b Compare November 22, 2024 14:00

AdrianSosic force-pushed the refactor/allow_flags branch from 818741b to 1d23a9d Compare November 22, 2024 20:17

AVHopp reviewed Nov 25, 2024

View reviewed changes

baybe/utils/basic.py Show resolved Hide resolved

AVHopp reviewed Nov 25, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

baybe/utils/basic.py Show resolved Hide resolved

AVHopp reviewed Nov 26, 2024

View reviewed changes

Scienfitz approved these changes Nov 26, 2024

View reviewed changes

AdrianSosic force-pushed the refactor/allow_flags branch from b7a3cea to 1766f8e Compare November 26, 2024 20:49

AdrianSosic added 9 commits November 26, 2024 22:33

Move allow_* flags to campaign class

71e42c3

Update flag handling in tests

c65d508

Update flag handling in examples

4bfced7

Move flag description in user guide

52a6604

Move pending experiments flag to campaign class

396ff84

Update admonition mentioning pending experiments flag

24d3ef1

Remove pending experiments flag from tests

74206d8

Rename _annotated.py to _filtered.py

94ec960

Remove exclude argument from get_candidates

4bdde65

AdrianSosic added 12 commits November 26, 2024 22:33

Draft FilteredSubspaceDiscrete class

ce18c58

Harmonize allow_* flags

f56eaf3

The original name allow_repeated_recommendations is not accurate since it also suggests that repeated configurations within a batch would be excluded, which is not the case.

Add deprecation mechanism for allow_* flags

17a858e

Implement context-aware validation of allow_* flags

087abf8

Fix flag handling in tests

4a0f56b

Add TODO note

865813a

Drop pending measurements from search space before recommending

b9fa97b

Update CHANGELOG.md

805649c

Drop unnecessary parts from changelog

ffe2b27

Mention possible relaxations when not enough candidates remain

1fef6da

Consider both Boolean values for flag test

c6ec66f

AdrianSosic force-pushed the refactor/allow_flags branch from 1766f8e to 048d5eb Compare November 26, 2024 21:34

Ignore missing sphinx reference

e366077

AdrianSosic force-pushed the refactor/allow_flags branch from 048d5eb to e366077 Compare November 26, 2024 22:23

Add BotorchRecommender to deprecation test

dfa7a70

AVHopp approved these changes Nov 27, 2024

View reviewed changes

AdrianSosic mentioned this pull request Dec 18, 2024

Pending experiments with CustomDiscreteParameter value not in search space gives cryptic error #453

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move `allow_*` flags to `Campaign` #423

Move `allow_*` flags to `Campaign` #423

AdrianSosic commented Nov 9, 2024 •

edited

Loading

AdrianSosic Nov 21, 2024

Scienfitz Nov 26, 2024

AdrianSosic Nov 26, 2024

AVHopp Nov 27, 2024

Scienfitz Nov 27, 2024 •

edited

Loading

AdrianSosic Nov 27, 2024

Scienfitz Nov 29, 2024

Scienfitz Dec 9, 2024

AVHopp Dec 9, 2024

AdrianSosic Dec 9, 2024

AVHopp left a comment

AVHopp left a comment

AVHopp Nov 26, 2024

AVHopp Nov 26, 2024

Scienfitz Nov 27, 2024 •

edited

Loading

Scienfitz Nov 26, 2024

Scienfitz Nov 26, 2024

AdrianSosic Nov 26, 2024

AVHopp left a comment

Move allow_* flags to Campaign #423

Are you sure you want to change the base?

Move allow_* flags to Campaign #423

Conversation

AdrianSosic commented Nov 9, 2024 • edited Loading

Coming next

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Recommender Level

Campaign Level

Choose a reason for hiding this comment

Scienfitz Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AVHopp left a comment

Choose a reason for hiding this comment

AVHopp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Scienfitz Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AVHopp left a comment

Choose a reason for hiding this comment

Move `allow_*` flags to `Campaign` #423

Move `allow_*` flags to `Campaign` #423

AdrianSosic commented Nov 9, 2024 •

edited

Loading

Scienfitz Nov 27, 2024 •

edited

Loading

Scienfitz Nov 27, 2024 •

edited

Loading