Skip to content
New issue

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

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

Already on GitHub? # to your account

[Bug] filter_kwargs warning leads to unintended thrown exception, when using partial #1667

Closed
mrcslws opened this issue Feb 9, 2023 · 3 comments
Assignees
Labels
bug Something isn't working

Comments

@mrcslws
Copy link

mrcslws commented Feb 9, 2023

🐛 Bug

Currently if I run batch_cross_validation using partial(MyModel, ...) as my model_cls, botorch accidentally throws an exception. The exception happens during param checking, when it tries to print the __name__ of the partial. This code worked without throwing an exception until the recent changes to _filter_kwargs.

This warn happens by default with batch_cross_validation because this function passes in train_Yvar=None into the class, which many classes don't use (e.g. SingleTaskGP). You might consider this a second bug; batch_cross_validation should only pass in train_Yvar if it is not None.

To reproduce

Here's an isolated repro that doesn't require calling batch_cross_validation.

import functools

import botorch

class MyModel:
    def __init__(self, my_param1, my_param2):
        self.my_param1 = my_param1
        self.my_param2 = my_param2

# This works.
cls = MyModel
botorch.optim.utils._filter_kwargs(MyModel,
                                   my_param1=42,
                                   my_param2=43,
                                   my_param3=44)

# This used to work, but started failing recently.
cls = functools.partial(MyModel,
                        my_param1=42)
botorch.optim.utils._filter_kwargs(cls,
                                   my_param2=43,
                                   my_param3=44)

** Stack trace/error message **

Traceback (most recent call last):
    botorch.optim.utils._filter_kwargs(cls,
  File "/Users/mrcslws/dev/src/botorch/botorch/optim/utils/common.py", line 28, in _filter_kwargs
    f" not allowed parameters for function {function.__name__}. Allowed "
AttributeError: 'functools.partial' object has no attribute '__name__'

Expected Behavior

It should just print a warning, as the code intended. Instead, the printing code itself is hitting an error.

System information

botorch: 0.7.3.dev22+g208470e7.d20221102
gpytorch: 1.9.1.dev32+g23b068b5
torch: 1.13.1
MacOS M1

@mrcslws mrcslws added the bug Something isn't working label Feb 9, 2023
@Balandat
Copy link
Contributor

Thanks for the report. This is interesting, I've never seen partial being used with classes - seems a bit weird to me, borderline abusive (but then again a lot of python is borderline abusive by default).

To support this specific use case we could just check whether this is a partial and then check for its args on the func attribute that contains the class. This seems like a bit of a hack to accommodate this; but then again I see the value of using partial here.

An alternative would be to just subclass the model and write an __init__ method that fixes those inputs, but presumably that's way too clunky/verbose?

cc @esantorella as this seems to be a consequence of your recent changes.

@Balandat
Copy link
Contributor

You might consider this a second bug; batch_cross_validation should only pass in train_Yvar if it is not None.

Good point, will make a new issue for tracking this.

@esantorella
Copy link
Member

This is a great point, thanks for the bug report! This would also fail if functools.partial is applied to a function, e.g.

from scipy.optimize import minimize
from functools import partial

# raises AttributeError
partial(minimize, method="L-BFGS-B").__name__

I'll put in a fix!

esantorella added a commit to esantorella/botorch that referenced this issue Feb 14, 2023
…ut a `__name__` attribute

Summary: See pytorch#1667

Differential Revision: D43286116

fbshipit-source-id: 2cd830e8206774998f57302c69fb7078d33b4033
facebook-github-bot pushed a commit that referenced this issue Feb 15, 2023
…ut a `__name__` attribute (#1678)

Summary:
Pull Request resolved: #1678

See #1667

Reviewed By: danielrjiang

Differential Revision: D43286116

fbshipit-source-id: 3da3e6ff23b517f5379ee90f407dc04d4f2ad06e
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants