Skip to content

add model to __all__ #2854

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jduerholt
Copy link
Collaborator

Motivation

RobustRelevancePursuitSingleTaskGP is not listed in __all__ in bofire/models/__init__.py. This PR adds it.

Have you read the Contributing Guidelines on pull requests?

Yes.

Test Plan

Not needed.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label May 21, 2025
@jduerholt
Copy link
Collaborator Author

Hmm, one gets circular imports when one adds it, I assume this was the reason for not adding it in the first place ..., so I think, I should close this PR again, or what do you think?

@Balandat
Copy link
Contributor

Hmm, interesting. I though this would be fixable by changing the from botorch.models import SingleTaskGP in https://github.com/pytorch/botorch/blob/main/botorch/models/robust_relevance_pursuit_model.py#L40 to from botorch.models.gp_regression import SingleTaskGP but that in itself will cause a circular import.

@SebastianAment since you wrote this code, do you have a better sense of what the issue here is?

@jduerholt
Copy link
Collaborator Author

It is also not important at all. We just encountered that the RobustRelevancePursuitSingleTaskGP has to be imported in a different way than the other models and I thought it should be easily fixable so I fired up this PR without even running the tests locally ..., but this has no priority for us at all, we import now directly from the source file ...

@Balandat
Copy link
Contributor

Yeah - though the fact that things break even if you don't do this but instead adjust the import as in #2854 (comment) is disconcerting - we should understand what causes this and fix it.

@SebastianAment
Copy link
Contributor

It is also not important at all. We just encountered that the RobustRelevancePursuitSingleTaskGP has to be imported in a different way than the other models and I thought it should be easily fixable so I fired up this PR without even running the tests locally ..., but this has no priority for us at all, we import now directly from the source file ...

I suspect this is because I added the relevance pursuit model to the multiple dispatch of fit_gpytorch_mll. That file likely introduces the import issue, but haven't checked, currently on vacation.

@Balandat
Copy link
Contributor

Hmm yeah that would make sense. Let's take a look and see if that's the case when you're back; we may want to think about separating out the registries in a way that would avoid these circular import issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants