Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bandit dispatcher class #372

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Bandit dispatcher class #372

wants to merge 5 commits into from

Conversation

AdrianSosic
Copy link
Collaborator

This PR adds a MultiArmedBandit dispatcher class implementing the strategy design pattern.

@AdrianSosic AdrianSosic added the enhancement Expand / change existing functionality label Sep 6, 2024
@AdrianSosic AdrianSosic self-assigned this Sep 6, 2024
@AdrianSosic AdrianSosic marked this pull request as draft September 6, 2024 12:44


@define
class MultiArmedBanditSurrogate:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @AVHopp, @Scienfitz: here a first draft of what we've discussed. At this stage, a few comments:

  • In principle, this setup works already. However, I'm not yet 100% convinced if this is the best way to go. We should not rush this decision, in particular since the same question (i.e. how to dispatch to more specialized implementations) will soon also arise for our other surrogates, e.g. ApproximateGPs, SparseGPs, ...
  • While it works, it does have the disadvantage that you basically loose all autocomplete when you invoke the dispatcher, which is not great. Perhaps we can improve upon this somehow by making it inherit from a (possibly new) base class for all bandit models, where we define the interface? The problem is however, that we cannot inherit from Surrogate nor from SurrogateProtocol, since the attribute forwarding will get very ugly otherwise
  • Overall, looking at the code, if we decide to keep it or a similar version, I don't want to expose only the dispatcher. To me, this is more like a utility in the sense that a user has no clue what to use can simply throw their specifications in and will get a reasonable model choice in return. However, there is no reason not to allow also full control by additionally exposing the specialized classes.

What do you think so far? That said, we also don't need to rush with the decision, especially if we agree on the last point. Because than we can also add the dispatcher after the release.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re 2:
would it be possible to create a bandit protocol and inherit from it for that purpose? or does the autocomplete not work with it if its just protocol?

re 3:
I see your argument but it would somehow end up in this situation of us having lots of models laying around. so when I type from baybe.surrogates import or just search the docs etc about what models are available, I would get lots of models and one model that looks like a base class due to the naming. Even though its not a base class in our case Im always annoyed if packages also expose all the small detailed objects - it will perhaps confuse less senior users.

At the same time, I would argue the dispatcher offers full control. Eg, for users who know about the beta/bernulli/binary/bandit connection. They simply set of stuff with beta prior and binary target.

Lastly, imo it would be safer to release the special MAB as private, instead of the other way round -> should we reverse the decision no deprecation required

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The more I think about it, the more I'd go away again from the strategy design pattern again and rather use a factory approach. This would reduce one level of nesting, make typing/auto-complete more straightforward, and require no additional machinery. Related to that your question about protocols: we already have all we need, I think: there is the Surrogate base class and the SurrogateProtocol, and all specialized bandits are confirm with them. The MAB class on the other hand is NOT a Surrogate (it does not inherit from it and it shouldn't, because I'd then have all stuff twice). And while it sort of complies with the Protocol, it's only because we trick it to do so by forwarding attribute access, causing the typing issues. So I'm really not yet convinced...

While I might also overlook other issues with the alternative approach, it seems a bit clearer to me. It would mean to just replace the MAB class with, say, a factory function make_bandit that returns an object of type Surrogate. Done. The downsides are: people need explicitly call the factory, and they need to specify the domain specs upfront and later again. But when regarded only as a utility for automatic model selection as alternative to manually specifying the model, this is completely OK.

So anyway, I have no clear decision at the moment. Your choice to decide what to do 🙃 before you to, perhaps play around with it interactively and experience the feeling and typing

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the status here now? Should we discuss this in one of our upcoming meetings? Seems like this touches an important, general point that we should align on.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either that or in a next baybathon. I think it's an important and general design decision we should be very clear about, in case we want to add it. But since it's not pressing, perhaps a baybathon with some other collected topics for it would be nice. Whiteboard sessions are more fun than teams meetings 🙃

@AdrianSosic AdrianSosic marked this pull request as ready for review September 6, 2024 12:59
searchspace: SearchSpace, objective: Objective
) -> type[BetaBernoulliMultiArmedBanditSurrogate]:
"""Retrieve the appropriate bandit class for the given modelling context."""
match searchspace, objective:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

included the searchspace in this private (so not affected by deprecation considerations) function why?



@define
class MultiArmedBanditSurrogate:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re 2:
would it be possible to create a bandit protocol and inherit from it for that purpose? or does the autocomplete not work with it if its just protocol?

re 3:
I see your argument but it would somehow end up in this situation of us having lots of models laying around. so when I type from baybe.surrogates import or just search the docs etc about what models are available, I would get lots of models and one model that looks like a base class due to the naming. Even though its not a base class in our case Im always annoyed if packages also expose all the small detailed objects - it will perhaps confuse less senior users.

At the same time, I would argue the dispatcher offers full control. Eg, for users who know about the beta/bernulli/binary/bandit connection. They simply set of stuff with beta prior and binary target.

Lastly, imo it would be safer to release the special MAB as private, instead of the other way round -> should we reverse the decision no deprecation required

@Scienfitz Scienfitz marked this pull request as draft September 8, 2024 21:36
Copy link
Collaborator

@Scienfitz Scienfitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resetting review, the design apparently is far from over and should be thought through thoroughly because something similar is likely coming for GPs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Expand / change existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants