Simplify and extend meta recommender logic #457

AdrianSosic · 2025-01-10T14:08:20Z

This PR refines the semantics of Meta recommenders and extends their use. It was motivated by two problems:

First one arising in Move allow_* flags to Campaign #423 where pending_experiments should only be passed to BayesianRecommenders. Unfortunately, with the current meta recommender interface, it is is not straightforward (or impossible?) to identify what exactly the next recommender will be – which indicates a suboptimal design.
Second: fixes Unintuitive switching behavior of Meta-Recommenders in combination with Campaign #420

The important things changed:

MetaRecommenders now have a much simpler logic, i.e. they only contain a select_recommender, which always just returns the recommender appropriate for the context of the call. Previous statefulness, which was only relevant to sequential recommenders, has been moved to the corresponding classes.
They now have an is_stateful class variable
They now have an get_inner_recommender method to leave the meta level
They can now be composed of arbitrary other meta recommenders. The previous restriction that they need to be composed of pure recommenders was an unnecessary limitation. Allowing other meta recommenders as building blocks can indeed be indeed useful, for example, using a two-phase recommender where the second phase uses an adaptive (e.g. batch-size dependent) meta recommender.
Added the new BatchSizeAdaptiveMetaRecommender, which can be useful, e.g. to avoid too costly optimizations for large batch sizes.
TwoPhaseMetaRecommender now has an explicit remain_switch option to clarify its behavior.

Extract common logic into new base class

Requiring that they are composed of pure recommenders is an unnecessary limitation. Allowing other meta recommenders as building blocks can indeed be useful, for example, using a two-phase recommender where the second phase uses an adaptive (e.g. batch-size dependent) meta recommender

baybe/recommenders/meta/sequential.py

AdrianSosic · 2025-01-10T14:45:20Z

tests/test_meta_recommenders.py

+        (StreamingSequentialMetaRecommender, None),
+    ],
+)
+def test_sequential_meta_recommender(cls, mode):


I drafted this part before Christmas and only remember that it was a big brain fuck. Instead of going once again over it, I'd simply give it for you to review with a fresh mind 🙃 Perhaps you can find some logic flaw that I overlooked or a test condition that is missing. Potentially, the whole thing can also be implement much more elegantly? Don't know ...

I really don't like all of the manipulation happening here. Why not crafting a very simple explicit search space (or what ever is necessary) and actually do a few recommendations?

I don't see how your comment relates to the test, tbh. Even if there was a searchspace involved, the point of the test is to check if the recommendation is made by the correct recommender – the actual recommendation output does not matter. So what is tested here if the recommender selection is done correctly, not the recommendation itself. And for that, the search space is not relevant at all.

Let me formulate it that way: I would prefer an example that does not require you to explicitly manipulate stuff like _was_used by hand and that we do not need to "pretend" that stuff happens. I'd prefer a test where this actually happens.

My impression was that this would be achieved the easiest by having a "full" example including a search space.

Ok, now I understand what you mean. I agree with the _was_used part, I didn't like that either. But at the same time, we should take more care that our tests are actual unit tests, i.e. that they don't run unnecessary costly code. In particular, we should avoid the anti-pattern that we also have in our examples where you construct something and then – regardless of the limited context – you do a full campaign-recommend loop.

So probably the "correct" way to achieve best of both worlds is to separate the "caching" of the meta recommends from the actual selection logic, so that in the test, we can focus only on the latter without having to modify flags. Let me see if this is easily doable...

Don't get me wrong, I fully get your point and would also prefer to simplify tests as much as possible. I think we are on the same page here. Maybe we should also ask @Scienfitz for his opinion?

Tried to come up with a better design that wouldn't require manually setting was_used in the tests but I always ended up with a more complicated class in the end. So unless one of you has a concrete proposal that reduces overall complexity, I'd stick with the current approach, i.e. rather have one inelegant step in the test then in the class itself. Will leave open until fully reviewed

What do you mean with "more complicated class"? What class exactly are you talking about, why would you need to adjust a class for a test?

AdrianSosic · 2025-01-10T16:33:38Z

@AVHopp @Scienfitz
Note, I've just added one more commit introducing a get_inner_recommender method, which is useful for #423.

AVHopp

Incomplete review since there was another issues that required my attention, but still a first bunch of comments.

CHANGELOG.md

baybe/recommenders/meta/base.py

baybe/recommenders/meta/sequential.py

tests/validation/utils/test_partition_validation.py

AVHopp

Last bit of my review that was still missing.

baybe/recommenders/meta/adaptive.py

Scienfitz · 2025-01-17T16:20:59Z

baybe/recommenders/meta/base.py

-    MetaRecommender,
-    lambda x: unstructure_base(
-        x,
-        # TODO: Remove once deprecation got expired:


was the deprecation expired or why was this removed?

Scienfitz · 2025-01-17T16:25:09Z

baybe/recommenders/meta/sequential.py

+    """Determines if the recommender should remain switched even if the number of
+    experiments falls below the threshold value in subsequent calls."""
+
+    _has_switched: bool = False


why no attrs syntax for this and remain_switched?

Scienfitz · 2025-01-17T16:29:52Z

baybe/recommenders/meta/sequential.py

+        # If the training dataset size has decreased, something went wrong
+        if (
+            n_data := len(measurements) if measurements is not None else 0
+        ) < self._n_last_measurements:


where does this condition come from? Why is it not allowed to decrease data size? For 2phase you explicitly allow it and implement something

if this is simply our convention/definition if ssequential metarecommenders then it needs o be well mentioned and documented

Scienfitz · 2025-01-17T17:18:32Z

baybe/recommenders/meta/adaptive.py

+    """The recommenders for the individual batch size intervals."""
+
+    partition: Partition = field(
+        converter=lambda x: Partition(x) if not isinstance(x, Partition) else x


do we want to provide reasonable defaults for recommenders and partition ? Otherwise this class seems useless as most users have little clue how to set it up reasonably

Scienfitz · 2025-01-17T17:31:02Z

baybe/recommenders/meta/base.py

        objective: Objective | None = None,
        measurements: pd.DataFrame | None = None,
        pending_experiments: pd.DataFrame | None = None,
-    ) -> PureRecommender:
+    ) -> RecommenderProtocol:
        """Select a pure recommender for the given experimentation context.


docstring still says pure but i guess is should say non-meta?

Scienfitz · 2025-01-17T17:31:48Z

baybe/campaign.py

@@ -453,7 +460,13 @@ def get_surrogate(self) -> SurrogateProtocol:

        pure_recommender: RecommenderProtocol
        if isinstance(self.recommender, MetaRecommender):
-            pure_recommender = self.recommender.get_current_recommender()
+            pure_recommender = self.recommender.select_recommender(


is it guaranteed that this returns a pure recommender?

AdrianSosic added 18 commits January 10, 2025 15:07

Add test case for switching hysteresis

1fe2cad

Adjust logic to make the hysteresis test pass

2008ba7

Update __str__ method

36ef187

Make arguments to MetaRecommender.select_recommender optional

87ccd41

Add Partition class

8d0a840

Add BatchSizeControlledMetaRecommender class

42d038a

Remove current state logic of meta recommenders

b947bc2

Reimplement SequentialMetaRecommender logic and test

f70a7aa

Reimplement StreamingSequentialMetaRecommender

a684662

Extract common logic into new base class

Improve docstrings

0741be1

Adjust recommender retrieval in Campaign class

cae0869

Add class variable indicating statefulness of the recommender type

5b32fcb

Replace default factory method with inline factory

7aa3b14

Use None for unitialized step indicator

a799ae5

Rename to BatchSizeAdaptiveMetaRecommender

fba7167

Update CHANGELOG.md

4466b0f

Deprecate obsolete methods of MetaRecommender

281ad57

AdrianSosic added enhancement Expand / change existing functionality refactor labels Jan 10, 2025

AdrianSosic self-assigned this Jan 10, 2025

Add missing class docstring

7a9f123

AdrianSosic marked this pull request as ready for review January 10, 2025 14:33

AdrianSosic requested review from Scienfitz and AVHopp as code owners January 10, 2025 14:33

AdrianSosic marked this pull request as draft January 10, 2025 14:34

AdrianSosic commented Jan 10, 2025

View reviewed changes

AdrianSosic marked this pull request as ready for review January 10, 2025 14:45

Add get_inner_recommender method

7c980eb

AdrianSosic mentioned this pull request Jan 10, 2025

Move allow_* flags to Campaign #423

Open

AVHopp reviewed Jan 13, 2025

View reviewed changes

tests/validation/utils/test_partition_validation.py Show resolved Hide resolved

AVHopp reviewed Jan 14, 2025

View reviewed changes

baybe/recommenders/meta/adaptive.py Outdated Show resolved Hide resolved

baybe/recommenders/meta/adaptive.py Outdated Show resolved Hide resolved

AdrianSosic added 6 commits January 16, 2025 09:18

Make BaseSequentialMetaRecommender public

c427f39

Refine get_inner_recommender name and docstrings

40f4d71

Validate first interval of BatchSizeAdaptiveMetaRecommender

2d30912

Fix/improve docstrings

0af5c2c

Add hypothesis strategy for Partition

e7b929b

Rename attribute of StreamingSequentialMetaRecommender

33f0dd9

Scienfitz reviewed Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify and extend meta recommender logic #457

Simplify and extend meta recommender logic #457

AdrianSosic commented Jan 10, 2025 •

edited

Loading

AdrianSosic Jan 10, 2025

AVHopp Jan 13, 2025

AdrianSosic Jan 13, 2025

AVHopp Jan 14, 2025

AdrianSosic Jan 14, 2025

AVHopp Jan 14, 2025

AdrianSosic Jan 16, 2025

AVHopp Jan 17, 2025

AdrianSosic commented Jan 10, 2025

AVHopp left a comment

AVHopp left a comment

Scienfitz Jan 17, 2025

Scienfitz Jan 17, 2025

Scienfitz Jan 17, 2025

Scienfitz Jan 17, 2025

Scienfitz Jan 17, 2025

Scienfitz Jan 17, 2025

Simplify and extend meta recommender logic #457

Are you sure you want to change the base?

Simplify and extend meta recommender logic #457

Conversation

AdrianSosic commented Jan 10, 2025 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdrianSosic commented Jan 10, 2025

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

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

AdrianSosic commented Jan 10, 2025 •

edited

Loading