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

python >= 3.6, add scipy 1.2.0 to min requirements #1035

Merged
merged 19 commits into from
Sep 5, 2020

Conversation

wholmgren
Copy link
Member

@wholmgren wholmgren commented Aug 27, 2020

  • Closes remove tools._array_newton #972
  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries to docs/sphinx/source/api.rst for API changes.
  • Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels and Milestone are assigned to the Pull Request and linked Issue.

I was 98% done with this PR when I realized that scipy 1.2.0 was released within the last 2 years (Dec 17, 2018). That's about a year newer than our pandas dependency and 2 years newer than our numpy dependency. So, it's much more aggressive than we've traditionally been with our dependencies. I'm on the fence about if this is a good idea or not for 0.8.

Lots of failures because the requires_scipy decorator was covering for missing requires_tables decorators.

@wholmgren
Copy link
Member Author

Thoughts from any maintainers or users on adding a required dependency on a scipy package that's less than 2 years old?

@kandersolar
Copy link
Member

I think I'm okay with it. NEP-29 suggests this guideline for numpy versions to support: All minor versions of numpy released in the 24 months prior to the project, and at minimum the last three minor versions.

I'm not sure it makes sense to apply that guideline to other packages, but if it does: Looking at scipy on PyPI, there were only RC versions between 1.1.0 and 1.2.0. Because 1.1.0 was released May 5, 2018, 1.2.0 is the oldest minor version released in the last 24 months, so requiring 1.2.0+ is consistent with the 24-month part of the guideline. Also the latest minor version is 1.5.x, so it satisfies the second part as well.

@cwhanse
Copy link
Member

cwhanse commented Sep 3, 2020

I'm OK with adding a dependency on scipy 1.20

@mikofski
Copy link
Member

mikofski commented Sep 3, 2020

I'm OK too

@wholmgren wholmgren added this to the 0.8.0 milestone Sep 4, 2020
@wholmgren
Copy link
Member Author

wholmgren commented Sep 4, 2020

Thanks for the feedback. I'm convinced. Ready for review. A couple more things to address... that scipy decorator was doing a lot of work.

Copy link
Member

@mikofski mikofski left a comment

Choose a reason for hiding this comment

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

Wow! I can't believe we're finally getting rid of that array newton code! I'm not sure but I think instead of removing scipy from the extra-requirements, statsmodels got removed instead. Thanks for this!

setup.py Outdated
'pvfactors', 'scipy', 'siphon', 'statsmodels', 'tables',
'cftime >= 1.1.1'],
'optional': ['ephem', 'cython', 'netcdf4', 'nrel-pysam', 'numba',
'pvfactors', 'scipy', 'siphon', 'tables', 'cftime >= 1.1.1'],
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'pvfactors', 'scipy', 'siphon', 'tables', 'cftime >= 1.1.1'],
'pvfactors', 'siphon', 'statsmodels', 'tables', 'cftime >= 1.1.1'],

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, I also forgot to remove scipy from the doc requirements.

@wholmgren
Copy link
Member Author

wholmgren commented Sep 4, 2020

I struggled with a test failure caused by the following situation:

  1. the environment specified in requirements-py35.yml does not include version limits for scipy (or numpy or pandas)
  2. the conda solver pulls packages for statsmodels 0.9 and scipy 1.1.
  3. pip install -e . causes scipy to be upgraded to 1.4
  4. statsmodels 0.9 looks for functions that don't exist in scipy 1.4 --> ImportError

To fix it I moved scipy and statsmodels to the pip section of the conda file. This works because the solver can pull newer wheels from pypi instead of relying on old, incompatible python 3.5 packages from anaconda or conda-forge.

I think this can be merged when the rest of the tests pass.

@mikofski
Copy link
Member

mikofski commented Sep 5, 2020

Why don't we drop support for python 3.5? I believe according to nep-29 a major release in November 2020 would only support Python 3.7 and newer? Pandas-1.1.1 installation guide supports Python 3.6 and newer. I'm okay with dropping 3.5

@wholmgren
Copy link
Member Author

@mikofski I also thought about dropping python 3.5 instead of trying this work around. My initial thought was that the workaround is simple enough and the dependencies all support 3.5, so it's no big deal for us to continue supporting 3.5. On the other hand, it's nice to drop python versions in 0.x releases instead of 0.x.y releases, and it seems like 3.5 might become a burden in the next 3-6 months.

3.6 features that I care about for pvlib are 1. fstrings and 2. dictionaries that preserve insertion order (technically not a language feature until 3.7 and only a CPython implementation detail in 3.6, but that's probably irrelevant to 99.9%-100% of pvlib users). Those features would make development and testing somewhat easier.

So I started this as a slight lean against dropping 3.5 and now I'm a slight lean for dropping 3.5. Other opinions?

@wholmgren wholmgren changed the title add scipy==1.2.0 to min requirements python >= 3.6, add scipy 1.2.0 to min requirements Sep 5, 2020
@wholmgren
Copy link
Member Author

The 3.5 CI runs have also been failing a lot recently for some dependency packaging reasons, so let's drop it now.

The coverage failure can be ignored. Ok to merge?

Copy link
Member

@mikofski mikofski left a comment

Choose a reason for hiding this comment

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

This was a big lift Will, thank you. I didn't realize that Python had already dropped 3.5 officially so that's more support in favor of this move. So I guess saying pvlib supports Python-3 is enough to imply >=3.6. Right?

I remember reading a rant by a Python core dev working at RedHat that maintaining old versions actually does more disservice to users than forcing them to upgrade, bc they are more vulnerable to security holes and code rot, but I couldn't find the reference.

Anyway LGTM to merge. @cwhanse, @kanderso-nrel I'll merge it tonight if I don't hear any complaints. Ok?

@kandersolar
Copy link
Member

LGTM. Two things on the contributing page could get updated -- ctrl-F for "Code must be compatible with Python 3.5 and above." and "for compatibility with python < 3.6"

@wholmgren
Copy link
Member Author

I removed the comments and classifiers for the specific python versions because there are too many to keep track of (thanks @kanderso-nrel for yet another that I missed). We also neglected to add the 3.8 classifier to setup.py after we started explicitly supporting it. In any case I think describing the specific versions is a lot less relevant now that 2.7 is gone.

After this is merged I'll open a new PR with the results of pyupgrade --py36-plus.

@mikofski mikofski merged commit edea606 into pvlib:master Sep 5, 2020
@mikofski
Copy link
Member

mikofski commented Sep 5, 2020

Thanks @wholmgren

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

Successfully merging this pull request may close these issues.

remove tools._array_newton
4 participants