Make `GaussianProcessSurrogate` a factory by AVHopp · Pull Request #853 · emdgroup/baybe

AVHopp · 2026-07-03T15:19:01Z

This PR changes the way that GaussianProcessSurrogate works by effectively making it a factory that composes different GP components into a single GP.

This was made in preparation for new functionalities where the setting of one of the components (e.g. the kernel) would depend on how another component was being set (e.g. the mean). In the previous design, this would have not been possible (or would have effectively overloaded the class).

The following core changes were made:

There is a new private _GaussianProcessSurrogate which effectively has the role of the previous public class. One of the key differences is that it now assumes to already received resolved components, and then simply glues them together. As this is purely meant for internal use now, this class does not inherit from any of our surrogate interfaces
The GaussianProcessSurrogate now acts as a factory, where the resolving of the individual components is intended to happen within _fit. Currently, this is not yet done as there is nothing to resolve, but ideally, this should be the place where components for e.g. mean transfer or MFBO are resolved.
The user facing interface of the GaussianProcessSurrogate has not changed, so this change should be "invisible" to users. The only exception, and why this is mentioned as a breaking change, is the following change:
The components of the GaussianProcessSurrogate now allow and default to None. This was the most elegant solution that came to my mind regarding the question "How can we cleanly handle the case when a user provides a single component that might conflict with one of our dispatched components"?. This can then be done in the individual dispatching branches by is not None checks. Again, no such check is currently in place as there is no such situation yet. Technically, this is however a breaking change as a user creating a GaussianProcessSurrogate and then accessing one of the components would not get a None instead of the corresponding factory, as those factories are only resolved at runtime.

This is my proposal for the factory, following our discussion and some ideas I shared with @AdrianSosic . If you see better or alternative ways of implementing this, I am open to it.

Copilot

Pull request overview

This PR refactors GaussianProcessSurrogate into a factory-style surrogate that resolves GP component factories at fit time and delegates actual model construction/fitting to a new internal _GaussianProcessSurrogate, enabling future component-dispatch logic (e.g., MFBO / transfer-mean dependent resolution) without overloading the public class.

Changes:

Introduces _GaussianProcessSurrogate as an internal “resolved-components” model builder and shifts SingleTaskGP construction/fitting into it.
Changes GaussianProcessSurrogate component factory fields to allow/default to None, resolving defaults during _fit.
Documents the resulting (intentional) breaking change in CHANGELOG.md.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.

File	Description
`baybe/surrogates/gaussian_process/core.py`	Implements the factory pattern via `_GaussianProcessSurrogate` and updates component factory fields to resolve at fit time.
`CHANGELOG.md`	Adds a Breaking Changes entry describing the new `None` default semantics for factory fields.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

kalama-ai · 2026-07-07T14:06:07Z

+        criterion = fit_criterion_factory(
            context.searchspace, context.objective, context.measurements
        )



Hi @AVHopp , thanks for drafting this one. I will try to rebase my PR for the dispatching between the PositiveIndexKernel and IndexKernel (the one with the many possible configuration conflicts we discussed) on this one to see if I run into any problems with the current proposal for the factory.

Could you clarify where you expect validation of conflicting kernel configuration to happen precisely?

Example: a user selects the transfer-learning kernel via the TaskParameter enum, but later also configures a different task-aware kernel explicitly in GaussianProcessSurrogate. Where should that conflict be detected and resolved?

I would say that for now, having this at the beginning of the _fit function would be the place for that, as I think this is the earliest place where we actually have access to all information necessary to do that - namely the search space and what the user configured.

This class is intended to replace the previous public version of the class which will become a factory-like class. Consequently, the class now receives already resolved components and only has a single fit function. Since the function is not user facing and not intended to be used directly, it is not part of the inheritance hierarchy (which might change, depending on how this refactoring goes). It is so far not yet used.

This is in preparation for the dispatching that will happen in the factory once the logic is in place. At this point, the default will also be changed to `None``

Final steps to making a factory out of the class. Fit `_GaussianProcessSurrogate` within `_fit` and adjust access to fields of `_model` to go through `_inner`

This prevents the need to have special handling for this case in `to_component_factory` which should only be concerned with converting valid objects to factories resp. passing those through

This method simply returned the fitted `BoTorch` model. Since any access to the fitted model is done by the "outer" class, it is not necessary to have this method in the inner class

Without this, accessing the fields would yield `None`, which is technically a breaking change. We hence use the pattern of accessing via properties which also simplifies some other parts of the code

Scienfitz · 2026-07-10T09:58:37Z

+        repr=False,
+        validator=optional(is_callable()),
    )
-    """The factory used to create the kernel for the Gaussian process.


we have a slightly strange problem now: I see you moved these docstrings to the properties.

At first glanve it makes sense. But the doc then will say things like "Accepts this and that as input" for properties, which are read-only by the very definition of properties.

Will re-think and revisit the docstrings

Changed in cd9e3e4. Note that I also removed some of the links as those aren't rendered resp. available to Sphinx anyway as those entries are private.. Would also be open to just have the double-backticks for everything. Please resolve if satisfied.

which links have exactly are we talking about?

is there a reason why you moved away from the bullet list? asking because it rendered nicely in sphinx

Answer to both questions (more or less): Since those fields are now private, they will (as far as I am aware) not be rendered in the sphinx documentation anyway, so the fields are only for developers. But I will double-check, just to be sure.

Please take a look here: https://avhopp.github.io/baybe_dev/latest/_autosummary/baybe.surrogates.gaussian_process.core.GaussianProcessSurrogate.html#baybe.surrogates.gaussian_process.core.GaussianProcessSurrogate While it is true that the __init__ shows the fields, we do not see their docstrings as the fields are not being rendered due to them being private, which is different here: https://emdgroup.github.io/baybe/stable/_autosummary/baybe.surrogates.gaussian_process.core.GaussianProcessSurrogate.html#baybe.surrogates.gaussian_process.core.GaussianProcessSurrogate

Also, note how you can also see the links that are not working and seems to not be detected:

Think the crux lies in the fact that the we have this custom Public attributes and properties override, which potentially blocks the rendering in the __init__ method? It's correct that the field itself is not shown, but the constructor part should rendered... Can you try what happens if we remove the custom stuff from rst?

The links are not working because they are pointing to something that does not exist, could that be? I have no clue what components.mean.MeanFactory should actually be 😄

My thoughts:

I know that there once was a reason for designing it that way - I think stuff was being duplicated or not rendered properly or something like that, I can investigate

I don't know but this is the link that we currently have in our documentation, and the last change to that file was the dev/gp merge, so the incorrect link there is on you @AdrianSosic ;)

Yes, not blaming anyone else here, just pointing out that the links are wrong. Probably, this is some artifact from merge/rebase or similar. I'm just wondering why the linkcheck didn't alarm us? In any case, we should fix them and then they can stay 👍🏼

Bad news on the documentation of __init__ for attrs in sphinx: This is not supported and an open issue for quite some time, and also the reason for our original design: sphinx-doc/sphinx#10682

We might be able to hack something together ourselves using a custom hooks using AST's (in a different PR), or we could be pragmatic and enhance the docstrings of the properties. Alternatively, we could change the docstring of the class itself. Opinions?

Regarding the broken links: I might have found the issue why this has not been flagged before, but I need to investigate first. Will open an issue if I can figure it out precisely.

Scienfitz · 2026-07-10T10:04:13Z

    @override
    def _posterior(self, candidates_comp_scaled: Tensor, /) -> Posterior:
-        return self._model.posterior(candidates_comp_scaled)
+        assert self._inner is not None


I assume this is here bcause of mypy?

But sometimes mypy cannot inver the stiautionc orrectly and other times mypy correctly infers that this attribute might be None at point of calling (and correctly warns about that which should thus be fixed rather than ignored).

Is this the former or the later case here? Because if the attribute can actually be None then we need to handle it properly and not via simple assert

This is to satisfy mypy. The code logic will always guarantee that the inner model exists at this point in time, but I will double-check and share some details on the flow and reasoning here or in the code via comments.

In general, _inner is available after fitting, the same like model was before. I think that the use of model previously didn't require us to add assert statements due to the type hints for that field missing.

The one in to_botorch was indeed potentially problematic - it is a public function, so it could be called at any time. This now raises a suitable error if being called before the model has been fitted, see 2f6c805. For the _posterior, we are in exactly the same situation we were in before, just that the assert was not necessary before due to typing (if I understand it correctly). So please resolve if this makes sense to you.

Scienfitz · 2026-07-10T10:06:14Z

not related to any specific line, but do we need to expand/change serialiation tests and/or hypothesis tests here?

There are now more possible values for the class so the strat needs to reflect that.

And do the serialziaiton tett now cover all new variants or do they need to be exapnded?

Good question, will take a look.

I've added two new tests:

Backward compatibility for serialization: Created a dict on main and used this for a test in 876763f. Sort of a "deprecation test", not sure if we really need that, but would be fine to also simply get rid of that.

Dedicated hypothesis strategy and serialization tests in 6c4db85. This should hopefully cover everything, resolve if happy.

kalama-ai · 2026-07-10T15:12:21Z


+@define
+class _GaussianProcessSurrogate:
+    """Internal model builder for :class:`GaussianProcessSurrogate`.


Since this class does not inherit form Surrogate and has a differnent interface, should we maybe rename it?

This is a good question, hence also summoning @AdrianSosic and @Scienfitz here.

I'd say that from a technical/mathematical perspective, this still is a Surrogate. However, it does not agree with our Surrogate-code as it is not user facing and does not need to follow that convention. I think the core issue is that we are now in the situation where we have factory that we do not call a factory. Probably, the cleanest way would be to carefully (re-)design the current abstractions that we have into Surrogate and/or SurrogateFactory, but then we would also need to re-name this class and loose that this is a non-breaking change for users - or have the very weird situation where we have an alias GaussianProcessSurrogate for a GaussianProcessSurrogateFactory which implements a SurrogateFactory interface.

What we could do is use another term like Model instead of Surrogate for the internal class. This is technically still correct and avoids those problems, and we could then at a later point generalise this/create dedicated interfaces for this. Opinions?

we need to avoid spamming the code with new objects and protocols at any cost and only do that if its strictly the best option

What is the problem here? The new factory respects the SurrogateProtocol - that is what matters. It does not matter that its not a Surrogate, or where would that become a problem?

I think the issue is that the internal class is called _GaussianProcessSurrogate while not satisfying the SurrogateProtocol which feels weird - at least this is how I understood @kalama-ai . Fully agree to not spam and use protocols just for the sake of it, so I'd prefer to either keep as-is (given that the class is internal anyway) or rename such that the internal class does not use the word Surrogate

ah., i overlooked the _ thats in front and thougt this is on the public one - that opens more questions though

Both the factory and the intenral class must respect the protocol, if not its not just weird its wrong. If the returned object doe not respect the surrogate protocol thats a major problem becuase its used as a surrogate. If the factory does not repsect the protocol thats less of a problem but it would mean the requirement for this class-based approach (invisible injection of the factory mechanism) is not satisfied.

Why did you see the need to completely underive the internal class from Surrogate? Naively I would have expected that the internal class remains more or less the same as before and a new factory is just injected on top of it. Why did yous ee the need for such strong changes and in particular underived it from Surrogate?

IIrc, the issue was basically that a lot of the functionalities would be duplicated if both satisfy the protocol., and this looked very weird to me. Will re-invstigtae.

Can you elaborate on the point about the "returned object"? The .to_botorch function returns a SingleTaskGP, and there is actually no point where we ever "return" an instance of the inner _GaussianProcessSurrogate class. Also, everything the user would ever interact with (like posterior calls) behaves exactly the same, there is basically just one additional wrapper, so I do not see why the inner model needs to satisfy any protocol.

I have now changed the inner class such that it implements the SurrogateProtocol in 1bde632. To avoid the full duplication/make the inner class usable without the outer (i.e., to have it properly implement the protocol), I introduced a small helper function _fit_from_tensors which can be used by the GaussianProcessSurrogate class as well. Was not as much code duplication as I originally thought, so we can also keep it like this.

Ok, for the chance that everyone is gonna kill me now, I need to propose something radical:

@AdrianSosic is like: Let's kick the private class entirely

And then everyone else is like: What!?!? This was the whole purpose of the PR, then we'd be back to square one

But let me explain, now that the picture is much clearer. The situation how we got here

We need a mechanism to orchestrate components jointly

--> Let's build a higher-level object (aka GP factory)

--> Let's wrap that object around the current one

--> Let's use the same name so that no user-facing change happes

--> Let's make the inner one still obey the protocol

--> Tension, so need to add some additional helper methods

And I now propose 7, which is:
7) Realize that the added all attributes are None-default setting already gives us the path that is needed to achieve 1 (i.e. the entry point where we say please orchestrate).

I also didn't see it earlier, but when reading @AVHopp proposal and the debate in this thread, it struck my eye.

So if you look in particular at:

The diff introduced by the last commit, you'll see that the code is muuuuch shorter, i.e. no additional class, no question about obeying protocols, no new helper etc

The diff to the original state, you'll see that all that has really changed is the added None path, which is exactly what we asked for initially.

So here it is: 916ed06
Hate or approve?

I have several things to say about that, but let's put this discussion into our dev meeting.

AVHopp · 2026-07-14T08:54:27Z

+        """The kernel factory used during model fitting."""
+        return self._kernel_factory or BayBEKernelFactory()
+
+    @property


THink about getting rid of the properties

Get rid of them, you can only resolve once you have context

Since components need to be resolved first, we cannot use properties to access them, as resolving the components cannot be done individually

AVHopp requested a review from kalama-ai July 3, 2026 15:19

AVHopp self-assigned this Jul 3, 2026

AVHopp requested a review from Scienfitz as a code owner July 3, 2026 15:19

Copilot AI review requested due to automatic review settings July 3, 2026 15:19

AVHopp requested a review from AdrianSosic as a code owner July 3, 2026 15:19

AVHopp added enhancement Expand / change existing functionality new feature New functionality labels Jul 3, 2026

Copilot started reviewing on behalf of AVHopp July 3, 2026 15:19 View session

Copilot AI reviewed Jul 3, 2026

View reviewed changes

AVHopp commented Jul 6, 2026

View reviewed changes

Comment thread CHANGELOG.md

AVHopp marked this pull request as draft July 6, 2026 09:15

kalama-ai reviewed Jul 7, 2026

View reviewed changes

AVHopp added 4 commits July 9, 2026 16:25

Allow None as for components

c7dcef9

This is in preparation for the dispatching that will happen in the factory once the logic is in place. At this point, the default will also be changed to `None``

Make a factory out of GaussianProcessSurrogate

a8fccd1

Final steps to making a factory out of the class. Fit `_GaussianProcessSurrogate` within `_fit` and adjust access to fields of `_model` to go through `_inner`

Update CHANGELOG

b2b38f1

AVHopp force-pushed the feat/gp_factory branch from fdfa715 to b2b38f1 Compare July 9, 2026 14:28

AVHopp added 4 commits July 9, 2026 16:51

Use lambda in converter to directly return None when passing None

2dbde9c

This prevents the need to have special handling for this case in `to_component_factory` which should only be concerned with converting valid objects to factories resp. passing those through

Only assign _inner after succesful fit

d0e3c3e

Remove to_botorch method from inner _GaussianProcessSurrogate class

6dd2b46

This method simply returned the fitted `BoTorch` model. Since any access to the fitted model is done by the "outer" class, it is not necessary to have this method in the inner class

Access fields via properties

6e923df

Without this, accessing the fields would yield `None`, which is technically a breaking change. We hence use the pattern of accessing via properties which also simplifies some other parts of the code

AVHopp marked this pull request as ready for review July 9, 2026 15:20

Scienfitz requested changes Jul 10, 2026

View reviewed changes

kalama-ai reviewed Jul 10, 2026

View reviewed changes

AVHopp added 6 commits July 13, 2026 11:12

Improve docstrings on fields and properties

cd9e3e4

Improve outdated docstring

ba98934

Introduce proper type hints for model

9f88efb

Replace assert by proper error handling

2f6c805

Add test for backward compatibility

876763f

Add hypothesis strategies and serialization tests

6c4db85

AVHopp and others added 3 commits July 13, 2026 13:09

Use optional_c mechanism to improve handling of None case

b9ce015

Implement SurrogateProtocol in inner class

1bde632

Drop inner _GaussianProcessSurrogate class

916ed06

AVHopp commented Jul 14, 2026

View reviewed changes

AVHopp added this to the 0.16.0 milestone Jul 14, 2026

AVHopp added 7 commits July 15, 2026 09:47

Make fields public and remove properties

4694fad

Since components need to be resolved first, we cannot use properties to access them, as resolving the components cannot be done individually

Move standard resolution path into a function

21f3f3b

Dispatch standard fit

1010f6c

Add test for accessing fields

eff23d0

Update CHANGELOG

06b3f4d

Add stub for MFBO dispatching

c946a94

Add stub for Mean Transfer dispatching

dc6f0b0

Uh oh!

Conversation

AVHopp commented Jul 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Scienfitz Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Scienfitz Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Scienfitz Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AdrianSosic Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Scienfitz Jul 10, 2026 •

edited

Loading

Scienfitz Jul 13, 2026 •

edited

Loading

Scienfitz Jul 13, 2026 •

edited

Loading

AdrianSosic Jul 13, 2026 •

edited

Loading