[MRG] Deprecate any import that is not from sklearn.neural_network #14913
Conversation
| # We don't want pytest to run these files since they immediately raise a | ||
| # DeprecationWarning | ||
| collect_ignore_glob = [ | ||
| "sklearn/neural_network/rbm.py", # 0.24 |
There was a problem hiding this comment.
does this also result in the doctests not to run?
There was a problem hiding this comment.
Yes, maybe rather not raise the warning in a pytest session?
Something like ,
import sys
if not getattr(sys, '_is_pytest_session', False):
warnings.warn(...)in the affected files, where sys._is_pytest_session is already set in conftest.py
There was a problem hiding this comment.
There is no docstring in rbm.py. There's almost no code in these files now, all they do is raises a warning.
@rth I have no strong opinion but I think that we should rather have pytest ignore a file, rather than have a file have specific behavior for pytest. LMK.
There was a problem hiding this comment.
There is no docstring in rbm.py. There's almost no code in these files now, all they do is raises a warning.
For now. Hard to to know some time later that this file is ignored by pytest just by looking at it (or to remove that line once the file is removed). I think if we can do deprecations locally without touching global config files it is better.
There was a problem hiding this comment.
For now
I don't see the file evolving, ever? This is the file we just created.
We only need to ignore the file because we run the tests with -Werror::DeprecationWarning. Not raising the warning when we are in a pytest session is treating the issue in the reverse order IMO, but OK. I'll address it in #14939 if you don't mind
There was a problem hiding this comment.
I don't see the file evolving, ever? This is the file we just created.
Me neither, but who knows. Accidents and messed up merge conflicts do happen.
Not raising the warning when we are in a pytest session is treating the issue in the reverse order IMO, but OK.
Yes, I agree it's not ideal, but both this and changing the conftest.py is a way around our non standard use of error on warning. Not sure if there is a better solution.
|
Thanks for the feedback!
I used I would like to do the same for all modules, please comment on #12927 (comment) ;) |
Any lint issue that used to be in I'd rather not fix the lint issues since it would be pretty much impossible to catch the changes in the diff (and there's a minimal chance that things can go wrong?). |
+1 |
|
I've opened #14939, maybe we can merge that one instead? It is more generic and will help deprecate the rest of the modules. But overall it does exactly the same job |
Sure. Closing this one then to avoid confusing reviewers :) |
Addresses part of #12927 and part of #9250
This PR renames
file.pyinto_file.pyin thesklearn.neural_networkmodule.Imports from
sklearn.neural_network.fileare still possible, but deprecated. The message explicitly says to only import tools fromsklearn.neural_network.The diff looks much more impressive that it actually is, due to file renaming.
From #12927 I understand that @adrinjalali @glemaitre @GaelVaroquaux @amueller are happy with this change. Ping also @thomasjpfan
ping @rth in particular because I think you had concerns about doing this.
The main result is that anything not in
sklearn/neural_network/__init__.pywill now be private!!(lint issue should be ignored.)