MAINT Update dependency versions #98
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 7 commits
June 21, 2019 13:55
The `logsumexp` function has moved in newer versions of `scipy`.
Addresses the following vulnerabilities in tensorflow: CVE-2018-7576, CVE-2018-7575, CVE-2019-9635, CVE-2018-7577, CVE-2018-10055, CVE-2018-7574. They are of severity: 1x CRITICAL, 3x HIGH, 2x MEDIUM.
The `scikit-learn` `check_estimator` was failing on a prediction check. Adjusting the number of epochs down seems to help.
Pass `flake8`; the method body was over-indented.
When installing from PyPI, pip will use the `install_requires` in `setup.py` and not `requirements.txt`. We should test with that. Also, Python 3.7.
Scikit-learn checks that model outputs are good; the model needs enough capacity and training time to properly hit the targets.
This is passing locally but still failing on Travis. (Why? It doesn't seem like this check should be failing.) Adjust parameters back to defaults, which also pass locally. Stop fixing `random_state`; the scikit-learn tests do that for us.
stephen-hoover
force-pushed
the
update-version
branch
from
cdb740f
to
40c90f8
Compare
In `tensorflow` v1.14 and Python v3.7, one of `tensorflow`'s modules has a `0` in the `__warningregistry__` attribute. Scikit-learn's `check_estimator` attempts to `clear` this attribute, assuming that it's a dictionary. Patch in an empty dictionary so that the estimator checks complete successfully.
stephen-hoover
force-pushed
the
update-version
branch
from
40c90f8
to
3284d16
Compare
mheilman
reviewed
Jun 24, 2019
| # with a `0` in the __warningregistry__. Scikit-learn tries to clear | ||
| # this dictionary in its tests. | ||
| name = 'tensorboard.compat.tensorflow_stub.pywrap_tensorflow' | ||
| with mock.patch.object(sys.modules[name], '__warningregistry__', {}): |
There was a problem hiding this comment.
Can we not always mock this? Also, what's the error without this?
There was a problem hiding this comment.
I would need to add the mock backport as a dev dependency for Python 2 if I wanted to always mock it. Since it's not necessary, I wanted to avoid the extra dependency. I could patch only if we have mock, but I preferred to make it clear that the problem is very version-limited.
The error from each test_check_estimator function is
___________________________________________________________________________ test_check_estimator ___________________________________________________________________________
def test_check_estimator():
"""Check adherence to Estimator API."""
> check_estimator(MLPRegressorFewerParams)
muffnn/mlp/tests/test_mlp_regressor.py:71:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
../../anaconda3/envs/muffnn-dev/lib/python3.7/site-packages/sklearn/utils/estimator_checks.py:293: in check_estimator
check_parameters_default_constructible(name, Estimator)
../../anaconda3/envs/muffnn-dev/lib/python3.7/site-packages/sklearn/utils/estimator_checks.py:2146: in check_parameters_default_constructible
with ignore_warnings(category=(DeprecationWarning, FutureWarning)):
../../anaconda3/envs/muffnn-dev/lib/python3.7/site-packages/sklearn/utils/testing.py:375: in __enter__
clean_warning_registry()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
def clean_warning_registry():
"""Clean Python warning registry for easier testing of warning messages.
We may not need to do this any more when getting rid of Python 2, not
entirely sure. See https://bugs.python.org/issue4180 and
https://bugs.python.org/issue21724 for more details.
"""
reg = "__warningregistry__"
for mod_name, mod in list(sys.modules.items()):
if hasattr(mod, reg):
> getattr(mod, reg).clear()
E AttributeError: 'int' object has no attribute 'clear'
../../anaconda3/envs/muffnn-dev/lib/python3.7/site-packages/sklearn/utils/testing.py:766: AttributeError
The clean_warning_registry function is part of scikit-learn's tests.
| @@ -2,5 +2,5 @@ numpy~=1.14 | |||
| scipy~=1.0 | |||
| scikit-learn~=0.19 | |||
| six~=1.10 | |||
| tensorflow~=1.4 | |||
There was a problem hiding this comment.
wouldn't this lead to the latest 1.x being installed already?
would tensorflow~=1.12.1 or tensorflow>=1.12.1,<=2.0.0 or something be better, to avoid potential incompatibilities?
There was a problem hiding this comment.
Yes, in practice we were using it with a newer version of tensorflow than this. The requirement change is to ensure that installations of muffnn avoid some security vulnerabilities in tensorflow.
I updated the requirements to require <2. In practice, I think we'll become incompatible before that, but I'm sure we would be incompatible in v2.
added 2 commits
June 24, 2019 12:27
We're very unlikely to be compatible with any v2 of tensorflow.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR increases the
tensorflowversion requirement to addresses the following security vulnerabilities in tensorflow: CVE-2018-7576, CVE-2018-7575, CVE-2019-9635, CVE-2018-7577, CVE-2018-10055, CVE-2018-7574. They are of severity: 1x CRITICAL, 3x HIGH, 2x MEDIUM. (From PR #97 .) Aside from increasing the requiredtensorflowversion and adding Python 3.7 testing, the rest of this PR is all modifications to the testing code to get tests to pass. Failures seem to be due to changes inscikit-learn,scipy, andflake8.