Skip to content

Conversation

@mdeceglie
Copy link
Collaborator

This is a patch to update several requirements. It addresses several security alerts raised by github (bleach, Jinja2, notebook) as well as updates several other packages to make installing requirements.txt and notebook_requirements.txt via pip run smoothly.

I propose merging this patch directly to master. Would appreciate a couple of sets of eyes on it.

cdeline
cdeline previously approved these changes Mar 26, 2020
Copy link
Collaborator

@cdeline cdeline left a comment

Choose a reason for hiding this comment

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

OK, this is good. Master should stay Pandas < 1.0 at the moment until we get our development branch merged which has pandas > 1.0.2 compatibility. Let's merge this so we can avoid getting Bleach security update emails. :)
Wait - nevermind. TravisCI is failing...

@cdeline cdeline dismissed their stale review March 26, 2020 15:25

Checks are failing

@abshinn
Copy link
Collaborator

abshinn commented Mar 26, 2020

Right, the Python 2 tests fail and error:

  • The "explicit requirements test" fails because Numpy 1.17.3 is not available for Python 2.7 via pip. The last supported Numpy version for Python 2.7 is 1.16.6.
  • The "eager requirements test" is able to go further because it installs Numpy 1.16.6, but ultimately errors on another Python 2.7 compatibility issue. Appears to be caused by statsmodels version being 0.11, which does not support Python 2.7.

If we want to maintain Python 2 support for a little longer, we could potentially split requirements between Python versions. Or, we can end Python 2 support with the next release.

If we want to go with the latter, perhaps it would make sense to not make this a hotfix -- rather make it a new release, update our version, make it explicit in our documentation.

@kandersolar
Copy link
Member

Strictly speaking, I'm not sure that updating the suggested package versions in requirements.txt is a breaking change since setup.py still allows older versions, but I think we should treat it like one anyway. Note that the development branch has already dropped 2.7: #135.

Is it possible to relax these updates a bit to accomplish the goals of this PR but maintain 2.7 compatibility in master until the next major release?

@abshinn
Copy link
Collaborator

abshinn commented Mar 26, 2020

I believe keeping Python 2.7 compatibility in this PR would require Numpy 1.16.6 and Statsmodels 0.8.0 in requirements.txt. I'm not sure if this will silence security alerts.

@mdeceglie Can you provide the text of the security alerts, and provide more detail on the pip issues?

@mdeceglie
Copy link
Collaborator Author

Thanks everyone. Neither numpy nor statsmodels were the sources of security alerts, but I was running into all kinds of error messages trying to install some older packages. Let me take another look at things with the python 2 question in mind.

@mdeceglie
Copy link
Collaborator Author

Works with python 2.7 now. I think I can live with the python=2.7, REQ_ENV='--upgrade --upgrade-strategy=eager .' check failing. What do folks think?

@kandersolar
Copy link
Member

I'm okay with the current state of this PR -- the usage docs recommend using requirements.txt so I think it's not a big deal to not support eager upgrades. Will this just live on master to silence the GH warnings, or will this be release 1.2.3?

@mdeceglie
Copy link
Collaborator Author

Since we modified setup.py i think it is worth a new release of the patch. While we might be able to get away with the old version of setup.py, the minimum requirements wouldn't have been tested (by Travis at least) and there is a known bug with pandas 1.0.0, so it makes sense to avoid that.

@cdeline @abshinn Let me know if you have any more input or would like more time to take a look. Otherwise I will proceed with the merge on 4/1.

Thanks!

Copy link
Collaborator

@abshinn abshinn left a comment

Choose a reason for hiding this comment

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

Thanks, @mdeceglie. I think this is good to merge.

@mdeceglie
Copy link
Collaborator Author

Thanks everyone. Proceeding with the merge. I will hold of on tagging a release because I want to look into fixing the same issue as #153 in the next patch.

@mdeceglie mdeceglie merged commit 52c426e into master Apr 2, 2020
@mdeceglie mdeceglie deleted the patch_requirements branch April 12, 2020 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants