search

cornellius-gp / gpytorch

A highly efficient implementation of Gaussian Processes in PyTorch
MIT License
3.46k stars 546 forks source link

Skip the warning in `gpytorch.lazy.__getattr__` if name starts with `_` #2423

Closed saitcakmak closed 8 months ago

saitcakmak commented 8 months ago

The current setup leads to the unittest self.assertWarns... to wrongly trigger a warning when entering _AssertWarnsContext (calls getattr on all modules: https://github.com/python/cpython/blob/main/Lib/unittest/case.py#L292-L294). Filtering out based on name starting with _ eliminates the needless warning and keeps the correct behavior.

esantorella commented 8 months ago

Hm, this seems like a very roundabout way of preventing this warning from being raised in an _AssertWarnsContext. Could the check be changed from if not name.startswith("_") to if name != "__warningregistry__"? Failing that, it would be good to have a comment explaining why the underscore check exists.

Or maybe the warning should be somewhere else? Perhaps in deprecated_lazy_tensor or deprecated_lazy_tensor.__init__, so it's only raised when a LazyTensor is actually instantiated?

saitcakmak commented 8 months ago

I left this check intentionally general since there may be other uses of getattr that we are not aware of, leading to the same warning elsewhere.

Or maybe the warning should be somewhere else? Perhaps in deprecated_lazy_tensor or deprecated_lazy_tensor.init, so it's only raised when a LazyTensor is actually instantiated?

Yeah, that could work. It is a bit of a trade-off between raising the warning at import vs instantiation time. Assuming interactive development: import time warning says "don't use this thing" before you start using it; instantiation time warning says "go back and change this to the correct class", which is a bit more work.

These have been deprecated for over a year by now, so the right move might be to reap them altogether.

Balandat commented 8 months ago

These have been deprecated for over a year by now, so the right move might be to reap them altogether.

It'd be nice to get rid of the lazies and this special handling - cc @gpleiss

saitcakmak commented 8 months ago

Should we land this and follow up with full deprecation later?

gpleiss commented 8 months ago

Oops sorry for the slow reply. I think it makes sense to land this, and then we can go with full deprecation later.

In general, we should do a deprecation sweep and actually get rid of all of the things marked as deprecated. I think everything marked has deprecated has been deprecated for over a year at least.