Skip to content
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

Update requirements.txt and setup.py according to GHSA-977j-xj7q-2jr9 #101

Merged
merged 2 commits into from
Apr 6, 2020

Conversation

timball
Copy link
Contributor

@timball timball commented Jan 29, 2020

This patch addresses the tensorflow vulns.

Copy link
Contributor

@stephen-hoover stephen-hoover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timball , thank you for the vulnerability notification! I see that the Tensorflow team has backported the patch to v1, and all the failing tests here tell me that muffnn isn't compatible with v2. Let's try increasing the Tensorflow version requirement but keeping it at v1.

requirements.txt Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@stephen-hoover
Copy link
Contributor

@timball , I don't see any additional changes. Why did you assign back to me?

Also, there's one thing I missed last time -- this will need an entry in the changelog.

@timball
Copy link
Contributor Author

timball commented Feb 7, 2020

Okay @stephen-hoover sorry. didn't mean to passive-aggressive assign the ticket. I accepted your changes. Hope this works! Still totally UNTESTED.

--timball

@stephen-hoover
Copy link
Contributor

@jacksonllee , it looks like Tensorflow v1.15.2 doesn't support Python 3.4 or 2.7, which this repo is still testing in. Thoughts? When will you be ready to end 2.7 and 3.4 support in muffnn?

The >= 3.5 tests have a couple of odd failures in the tests which I haven't had time to look into yet.

@jacksonllee
Copy link
Contributor

@stephen-hoover TL;DR -- I'm leaning towards having this PR merge for now, so that this PR (and @timball ) aren't on the hook. Then the current muffnn maintainers will fix the master branch shortly (for the following two issues) and make a new release.

Re: Python 2.7 and 3.4, since Tensorflow v1.15.2 doesn't support them anymore and we need at least this version of Tensorflow for security, it looks like muffnn would have to stop supporting Python 2.7 and 3.4 in the next release (coming up in about a week). Probably not ideal, especially since we didn't have a cycle of deprecation, etc. But it looks like Tensorflow is the limiting factor here... The action items would be to (i) remove Python 2.7 and 3.4 in the Travis CI yaml config and (ii) document in the README.md what Python versions are supported (there's currently no mention at all).

Re: test failures for Python 3.5+, it looks like they have to do with small numerical discrepancies. I can fix them in a separate PR as well.

cc @mheilman in case you have thoughts

stephen-hoover
stephen-hoover previously approved these changes Feb 12, 2020
Copy link
Contributor

@stephen-hoover stephen-hoover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @jacksonllee , if you're okay with merging this, please approve as well.

@jacksonllee
Copy link
Contributor

@timball @stephen-hoover Thank you again for the help! @mheilman and I decided that we shouldn't merge PRs that would fail the builds, and so we're holding off this PR while investigating #102.

@stephen-hoover stephen-hoover dismissed their stale review February 12, 2020 22:44

We changed our minds -- don't merge anything that would break the build.

@jacksonllee jacksonllee removed their assignment Feb 13, 2020
@mheilman mheilman mentioned this pull request Apr 6, 2020
@mheilman mheilman self-assigned this Apr 6, 2020
@mheilman mheilman removed their assignment Apr 6, 2020
Copy link
Contributor

@jacksonllee jacksonllee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants