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

Run GH Actions CI for py3.6+ and keep Travis for 3.5 and 2.7 #3067

Merged
merged 22 commits into from
Dec 21, 2020

Conversation

lilyminium
Copy link
Member

@lilyminium lilyminium commented Dec 9, 2020

Fixes #3079
Branches off #3061 to fix #2768. As elegantly suggested by @IAlibay, this PR

  • ports fixes from develop to master
  • Keeps GH actions for pythons 3.6-3.8
  • Keeps Travis for pythons 2.7 and 3.5

In #3061 I moved Python 2.7 and 3.5 CI to GH actions. It is a bit clunky, unreadable, and potentially difficult to maintain. Given that we can hopefully just move onto 2.0 and Python 3.6+, perhaps this compromise gets things done for an intermediate 1.0.1 release while we iron out the kinks on #3061.

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

lilyminium and others added 10 commits December 9, 2020 12:49
* fix MDAnalysis#3006
* fixed the for/else loop that accidentally added too many versions of dev/ and stable/

(cherry picked from commit 2d14dbd)
Not an issue targetted fix.
Improve CI (no user facing changes)

- Overview

  mamba is a dropin replacement for the conda CLI but faster

- Work done in this PR

  Enabled the `MAMBA` environment variable keyword for the CI helper.

(cherry picked from commit 2ee4e9d)
* increase default Python to 3.7 on Travis CI
   - set PYTHON_VERSION=3.7
   - set MAMBA=false for Python 3.5 (no mamba package available, fall backto conda)
* ensure conda package is available for numpy 1.13
   NumPy 13.3.3 packages are available for Python 2.7 and 3.6 only so for the tests with the
   minimal numpy version we set Python to 3.6.
Fixes MDAnalysis#3036 

## Work done in this PR
- Adds a continuous integration workflow based on github actions.
- Disables previous continuous integration workflow based on TravisCI.
- Fixes minor Visibledeprecation warnings with the hole2 tests.
Fixes MDAnalysis#3046. Deploys docs in gh-ci.yaml script and removes Travis
* TST, CI: add ARM64 Graviton 2 to CI

* start testing MDAnalysis on ARM64 using
new AWS Graviton 2 architecture available
in Travis CI

* testing on my fork shows only two failures
in the MDAnalysis test suite using a fairly
minimal set of dependencies; we can probably
either fix those or skip them and open an issue

* the test failures are:
`test_written_remarks_property`
`TestEncoreClustering.test_clustering_three_ensembles_two_identical`

* run time for this CI entry is about
23 minutes, which is solid for ARM; we save a lot
of time using experimental upstream wheels in some
cases and excluding (source builds of) dependencies
where possible until the ARM64 binary wheel ecosystem
matures a bit more

* MAINT: ARM64 shims

* adjust `CAffinityPropagation()` C-level function to use `double`
instead of `float` in some strategic locations that allow the
`test_clustering_three_ensembles_two_identical()` test to pass
on gcc115 ARM64 machine

* mark `test_written_remarks_property()` as a known failure on
ARM64; it fails in Travis CI on that platform, but not on gcc115
ARM64 machine; that said, this test is already known to be flaky
on Windows 32-bit (may depend on character settings on machine?)

* MAINT: MR 2956 revisions

* reduce optimization level of MDAnalysis builds on
ARM64 to avoid problems with `ap.c` causing test failures
on that architecture

* revert any source changes to `ap.c`
…2953)

* fix NameError when catching NoDataError
* add test + missing imports
…DAnalysis#3029)

Fixes MDAnalysis#3028

* Improves the performance of the ParmEd converter by using a dictionary lookup for the atomgroup to universe index mapping.
@pep8speaks
Copy link

pep8speaks commented Dec 9, 2020

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

Line 275:80: E501 line too long (81 > 79 characters)
Line 278:80: E501 line too long (81 > 79 characters)

Line 323:21: E127 continuation line over-indented for visual indent

Comment last updated at 2020-12-14 22:22:43 UTC

@lilyminium lilyminium requested a review from IAlibay December 9, 2020 02:03
@codecov
Copy link

codecov bot commented Dec 9, 2020

Codecov Report

Merging #3067 (09bdc9a) into master (2ef90f2) will increase coverage by 0.72%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3067      +/-   ##
==========================================
+ Coverage   91.46%   92.19%   +0.72%     
==========================================
  Files         173      166       -7     
  Lines       24203    22729    -1474     
  Branches     3193     3194       +1     
==========================================
- Hits        22138    20954    -1184     
- Misses       1438     1683     +245     
+ Partials      627       92     -535     
Impacted Files Coverage Δ
package/MDAnalysis/coordinates/ParmEd.py 88.26% <100.00%> (+4.83%) ⬆️
package/MDAnalysis/analysis/hole2/hole.py 73.40% <0.00%> (-8.78%) ⬇️
package/MDAnalysis/analysis/base.py 95.60% <0.00%> (-4.40%) ⬇️
package/MDAnalysis/topology/tpr/obj.py 96.82% <0.00%> (-3.18%) ⬇️
...onality_reduction/DimensionalityReductionMethod.py 97.14% <0.00%> (-2.86%) ⬇️
package/MDAnalysis/coordinates/chemfiles.py 90.47% <0.00%> (-1.20%) ⬇️
coordinates/__init__.py
analysis/__init__.py
util.py
auxiliary/__init__.py
... and 109 more

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 2ef90f2...09bdc9a. Read the comment docs.

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

Might just be me looking in the wrong place, but I can't seem to be able to see the py3.5 run?

@lilyminium
Copy link
Member Author

lilyminium commented Dec 9, 2020

@IAlibay I can't see a successful python 3.5 in https://travis-ci.com/github/MDAnalysis/mdanalysis/builds/192419679, on master a month ago?

Edit: nevermind, it was the blank one -- evidently before @orbeckst increased the Python to 3.7

@lilyminium lilyminium requested a review from IAlibay December 10, 2020 01:00
@IAlibay
Copy link
Member

IAlibay commented Dec 14, 2020

@lilyminium could you also port #3073 here too? (i.e. just delete the appveyor yaml file). That would save us having to open a separate PR to deal with this.

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

lgtm! thanks :)

Unless there's any major objections to this pseudo-port approach (i.e. having a small reliance on Travis credits for a branch that we don't often push to), then let's go with this (pinging @MDAnalysis/coredevs).

@IAlibay IAlibay mentioned this pull request Dec 14, 2020
2 tasks
IAlibay and others added 9 commits December 15, 2020 08:06
Fixes MDAnalysis#3036 

## Work done in this PR
- Adds a continuous integration workflow based on github actions.
- Disables previous continuous integration workflow based on TravisCI.
- Fixes minor Visibledeprecation warnings with the hole2 tests.
Fixes MDAnalysis#3046. Deploys docs in gh-ci.yaml script and removes Travis
* TST, CI: add ARM64 Graviton 2 to CI

* start testing MDAnalysis on ARM64 using
new AWS Graviton 2 architecture available
in Travis CI

* testing on my fork shows only two failures
in the MDAnalysis test suite using a fairly
minimal set of dependencies; we can probably
either fix those or skip them and open an issue

* the test failures are:
`test_written_remarks_property`
`TestEncoreClustering.test_clustering_three_ensembles_two_identical`

* run time for this CI entry is about
23 minutes, which is solid for ARM; we save a lot
of time using experimental upstream wheels in some
cases and excluding (source builds of) dependencies
where possible until the ARM64 binary wheel ecosystem
matures a bit more

* MAINT: ARM64 shims

* adjust `CAffinityPropagation()` C-level function to use `double`
instead of `float` in some strategic locations that allow the
`test_clustering_three_ensembles_two_identical()` test to pass
on gcc115 ARM64 machine

* mark `test_written_remarks_property()` as a known failure on
ARM64; it fails in Travis CI on that platform, but not on gcc115
ARM64 machine; that said, this test is already known to be flaky
on Windows 32-bit (may depend on character settings on machine?)

* MAINT: MR 2956 revisions

* reduce optimization level of MDAnalysis builds on
ARM64 to avoid problems with `ap.c` causing test failures
on that architecture

* revert any source changes to `ap.c`
…2953)

* fix NameError when catching NoDataError
* add test + missing imports
…DAnalysis#3029)

Fixes MDAnalysis#3028

* Improves the performance of the ParmEd converter by using a dictionary lookup for the atomgroup to universe index mapping.
@IAlibay
Copy link
Member

IAlibay commented Dec 21, 2020

@lilyminium this is probably good to merge. However, why is RTD failing?

@lilyminium
Copy link
Member Author

Thanks for the review @IAlibay! RTD is failing because #3078 isn't merged, because of #3079...

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.

9 participants