add h5py to setup.py's extra_requires #3711

orbeckst · 2022-06-09T01:05:35Z

Changes made in this Pull Request:

keep minimum version of h5py at >= 2.1.0
new 'extra_formats' target in extra_requires where we can add pip-installable packages for additional formats that are not installed by default
deprecated 'AMBER' target in extra_requires (now part of 'extra_formats')

PR Checklist

Tests? (not sure how to test)
Docs?
CHANGELOG updated?
Issue raised/referenced?

- fix #3701 - new 'extra_coordinates' target in extra_requires where we can add pip-installable packages for additional formats that are not installed by default - keep minimum version of h5py at >= 2.1.0

package/setup.py

orbeckst · 2022-06-09T01:06:58Z

package/setup.py

+              'extra_formats': [   # additional file formats
+                  'netCDF4>=1.0',  # for fast AMBER writing, also needs HDF5
+                  'h5py>=2.10',    # H5MD
+                  ],


possible add here

gsd (unless we just install it anyway)

chemfiles

gsd is an install requires for now so it shouldn't also be going into extra requires

chemfiles might be an option if we want

I added chemfiles>=0.10

orbeckst · 2022-06-09T01:07:19Z

package/setup.py

@@ -650,6 +643,10 @@ def long_description(readme):
              'AMBER': [


deprecated the AMBER target (not sure if anyone ever used it...)

package/setup.py

codecov · 2022-06-09T01:26:13Z

Codecov Report

Merging #3711 (afb49ee) into develop (cf17d42) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #3711   +/-   ##
========================================
  Coverage    94.30%   94.30%           
========================================
  Files          192      192           
  Lines        24695    24695           
  Branches      3327     3327           
========================================
  Hits         23289    23289           
  Misses        1358     1358           
  Partials        48       48

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf17d42...afb49ee. Read the comment docs.

IAlibay

oops comments weren't via review - I'll request changes over setting minimum versions for all the CI files please.

- GitHub actions - maintainer/conda/environment.yml for readthedocs - Azure pipelines

- include chemfiles>=0.10 in extra_formats - added comment to remove AMBER in 3.0.0

pep8speaks · 2022-06-09T21:29:28Z

Hello @orbeckst! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-07-05 19:28:49 UTC

orbeckst · 2022-06-10T19:31:09Z

I installed into an empty py39 env with

pip install ./package[extra_formats,analysis]

and packages import

Python 3.9.0 | packaged by conda-forge | (default, Nov 26 2020, 07:54:06)
[Clang 11.0.0 ] on darwin
Type "help", "copyright", "credits" or "license" for more information.
imp>>> import h5py
>>> import chemfiles
>>> h5py.__version__
'3.7.0'
>>> chemfiles.__version__
'0.10.2'
>>> import netCDF4
>>> netCDF4.__version__
'1.5.8'
>>> import MDAnalysis
>>> MDAnalysis.__version__
'2.3.0-dev0'

and after installing the testsuite, all tests pass

pytest  --disable-warnings  -n 4  testsuite/MDAnalysisTests

================= 17782 passed, 359 skipped, 2 xfailed, 2 xpassed, 32302 warnings in 315.77s (0:05:15) ==================

IAlibay

just the one thing, otherwise lgtm, thanks @orbeckst !

IAlibay · 2022-06-11T07:52:19Z

azure-pipelines.yml

@@ -92,7 +92,7 @@ jobs:
      pytest-xdist
      scikit-learn
      scipy
-      h5py
+      h5py>=2.10


Let's leave it as this for now, long term we might want to come back and put minimum pins on stuff here too (although to be honest we'll just pick up the maximum version here anyways, I guess I'm just being too pedantic...)

I understand your comment as "h5py>=2.10 is fine for now" so I am not changing it.

IAlibay · 2022-06-11T07:52:59Z

package/CHANGELOG

  * Deprecate `density` parameter in favor of `norm` in InterRDF_s
    (Issue #3687)
+  * The extras_requires target "AMBER" for `pip install ./package[AMBER]`
+    will be removed in 3.0. Use "extra_formats". (Issue #3701, PR #3711)


Suggested change

* Deprecate `density` parameter in favor of `norm` in InterRDF_s

(Issue #3687)

* The extras_requires target "AMBER" for `pip install ./package[AMBER]`

will be removed in 3.0. Use "extra_formats". (Issue #3701, PR #3711)

* The extras_requires target "AMBER" for `pip install ./package[AMBER]`

will be removed in 3.0. Use "extra_formats". (Issue #3701, PR #3711)

* Deprecate `density` parameter in favor of `norm` in InterRDF_s

(Issue #3687)

Newest entry first :)

Also I guess we shouldn't be a pain to the couple of folks that might use this, but does it really need to be a major version breaking change? The number of automated workflow using this should be near 0, and there's no way they'll get a deprecatewarning about this. If we enact the change the biggest issue is that things will just break cleanly right? (or will it just ignore the keyword and go ahead)

I don’t think it really needs to be a major revision deprecation. I was just very conservative. I’m not sure how many people used it, tbh.

i haven’t tried proving missing/incorrect target names. I assume it will break.

I’m happy to go with a deprecation for 2.4, too, just let me know or change directly in setup.py and CHANGELOG.

I set the deprecation for 2.4.

I tried passing the wrong extra (as 'analyses' instead of 'analysis')

WARNING: mdanalysis 2.3.0.dev0 does not provide the extra 'analyses'

So it won't fail if you give it the wrong thing (which is rather annoying). I'm still ok with setting the deprecation for 2.4 given the fact that we can't suitably warn users anyways, it'll be a problem either way.

IAlibay · 2022-06-11T08:52:59Z

Whilst we're here, do the changes here indicate that this is also fixed? #3406

Co-authored-by: Irfan Alibay <[email protected]>

orbeckst · 2022-06-11T15:58:43Z

Regarding #3406 I don’t know — we could add a restriction to not use h5py 3.4.0 but we would be guessing as we never pinpointed the ultimate cause. Ultimately we need someone to test if that failure is really only occurring with specific releases, then we exclude all bad ones here, and then I’d be comfortable closing the issue.

orbeckst · 2022-06-14T23:32:46Z

coverage-related error makes one test fail — no idea what to do about it; is this related to the need to pin coverage again that was mentioned somewhere on discord?

orbeckst · 2022-06-14T23:33:43Z

@IAlibay let me know if you think something else needs to be done here. No rush.

orbeckst · 2022-07-05T19:23:10Z

GitHub hangs on "Checking for ability to merge automatically…" —weird.

Anyway, @IAlibay is there anything else that needs doing?

IAlibay

"I'll give it a review once I'm back from the wilds of Edinburgh" oof I think I see why I didn't get around to re-reviewing this until this week. So sorry D: !

add h5py to setup.py's extra_requires

ff3996b

- fix #3701 - new 'extra_coordinates' target in extra_requires where we can add pip-installable packages for additional formats that are not installed by default - keep minimum version of h5py at >= 2.1.0

orbeckst commented Jun 9, 2022

View reviewed changes

IAlibay requested changes Jun 9, 2022

View reviewed changes

orbeckst added 3 commits June 9, 2022 14:21

minimum h5py >= 2.10

e104b32

- GitHub actions - maintainer/conda/environment.yml for readthedocs - Azure pipelines

update extra_deps in setup.py

6c4bd16

- include chemfiles>=0.10 in extra_formats - added comment to remove AMBER in 3.0.0

updated CHANGELOG

a7bc680

github-actions bot added the Continuous Integration label Jun 9, 2022

orbeckst added 2 commits June 10, 2022 11:59

pep8 fixes

ded6679

updated sklearn --> scikit-learn in setup.py

1762aad

IAlibay requested changes Jun 11, 2022

View reviewed changes

Update package/CHANGELOG

752890a

Co-authored-by: Irfan Alibay <[email protected]>

orbeckst mentioned this pull request Jun 11, 2022

H5PY 3.4.0 causes H5MD writer tests fail with PermissionError on Windows (investigate temporary pin) #3406

Closed

orbeckst and others added 2 commits June 11, 2022 16:49

deprecate extras_require "AMBER" target for 2.4

7bf94a9

Merge branch 'develop' into h5py-extradep

4a12625

orbeckst assigned IAlibay Jun 14, 2022

Merge branch 'develop' into h5py-extradep

78ae314

IAlibay approved these changes Jul 5, 2022

View reviewed changes

Merge branch 'develop' into h5py-extradep

afb49ee

orbeckst merged commit 2c32ed1 into develop Jul 6, 2022

orbeckst deleted the h5py-extradep branch July 6, 2022 08:58

IAlibay mentioned this pull request Aug 30, 2022

Remove AMBER entry in extra_requires #3810

Merged

2 tasks

IAlibay added the maintainability label Sep 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add h5py to setup.py's extra_requires #3711

add h5py to setup.py's extra_requires #3711

orbeckst commented Jun 9, 2022 •

edited

Loading

orbeckst Jun 9, 2022

IAlibay Jun 9, 2022

orbeckst Jun 10, 2022

orbeckst Jun 9, 2022

codecov bot commented Jun 9, 2022 •

edited

Loading

IAlibay left a comment

pep8speaks commented Jun 9, 2022 •

edited

Loading

orbeckst commented Jun 10, 2022

IAlibay left a comment

IAlibay Jun 11, 2022

orbeckst Jun 11, 2022

IAlibay Jun 11, 2022

IAlibay Jun 11, 2022

orbeckst Jun 11, 2022

orbeckst Jun 11, 2022

IAlibay Jun 13, 2022

IAlibay commented Jun 11, 2022

orbeckst commented Jun 11, 2022 •

edited

Loading

orbeckst commented Jun 14, 2022

orbeckst commented Jun 14, 2022

orbeckst commented Jul 5, 2022

IAlibay left a comment

		@@ -650,6 +643,10 @@ def long_description(readme):
		'AMBER': [

add h5py to setup.py's extra_requires #3711

add h5py to setup.py's extra_requires #3711

Conversation

orbeckst commented Jun 9, 2022 • edited Loading

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 9, 2022 • edited Loading

Codecov Report

IAlibay left a comment

Choose a reason for hiding this comment

pep8speaks commented Jun 9, 2022 • edited Loading

Comment last updated at 2022-07-05 19:28:49 UTC

orbeckst commented Jun 10, 2022

IAlibay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IAlibay commented Jun 11, 2022

orbeckst commented Jun 11, 2022 • edited Loading

orbeckst commented Jun 14, 2022

orbeckst commented Jun 14, 2022

orbeckst commented Jul 5, 2022

IAlibay left a comment

Choose a reason for hiding this comment

orbeckst commented Jun 9, 2022 •

edited

Loading

codecov bot commented Jun 9, 2022 •

edited

Loading

pep8speaks commented Jun 9, 2022 •

edited

Loading

orbeckst commented Jun 11, 2022 •

edited

Loading