Fix nightly NumPy cron job, add numpy 1.25-dev0 fixes #3977

IAlibay · 2022-12-20T07:21:20Z

Towards #3980

Changes made in this Pull Request:

Fixes cron job against nightly wheels
Fixes array comparison in streamlines_3D

PR Checklist

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

codecov · 2022-12-20T07:41:06Z

Codecov Report

Base: 93.48% // Head: 93.49% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (2f6f577) compared to base (0c4f823).
Patch coverage: 20.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3977   +/-   ##
========================================
  Coverage    93.48%   93.49%           
========================================
  Files          190      190           
  Lines        24951    24963   +12     
  Branches      3527     3527           
========================================
+ Hits         23326    23338   +12     
  Misses        1102     1102           
  Partials       523      523

Impacted Files	Coverage Δ
package/MDAnalysis/visualization/streamlines.py	`54.20% <0.00%> (ø)`
package/MDAnalysis/visualization/streamlines_3D.py	`70.49% <33.33%> (ø)`
package/MDAnalysis/core/groups.py	`97.51% <0.00%> (+0.02%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

IAlibay · 2022-12-20T10:03:59Z

Looks like we were having numpy conflicts between mamba and pip https://github.com/MDAnalysis/mdanalysis/actions/runs/3739353709/jobs/6346461409#step:6:195

IAlibay · 2022-12-27T22:50:23Z

right it's fixed, and we've actually picked up a breaking change with the nightly dev versions!

IAlibay · 2023-01-02T18:49:27Z

package/MDAnalysis/visualization/streamlines_3D.py

            cube_counter += 1

    def update_dictionary_end_frame(array_simulation_particle_coordinates, dictionary_cube_data_this_core):
        """Update the cube dictionary objects again as appropriate for the second and final frame."""
        cube_counter = 0
        for key, cube in dictionary_cube_data_this_core.items():
            # if there were no particles in the cube in the first frame, then set dx,dy,dz each to 0
-            if cube['centroid_of_particles_first_frame'] == 'empty':
+            if cube['centroid_of_particles_first_frame'] == None:


That took me far too long to narrow down, looks like the issue here was that we are at times doing an array vs str comparison, which until 1.24 led to a scalar return, but now returns an array of bools instead, that in turns messes up this if/else call.

Doing a comparison against None is probably just safer & more pythonic here?

P.S. Weirdly enough I think this should have thrown an error rather than silently failing i.e. it should have complained about the truth of an array being ambiguous.. but it didn't 😕

See https://github.com/MDAnalysis/mdanalysis/actions/runs/3824585204/jobs/6506856469 for a successful run.

IAlibay · 2023-01-02T19:19:11Z

@MDAnalysis/coredevs this should be good to go I think. Once it's merged I'll cherry pick the changes here and create a 2.4.2 bugfix release.

orbeckst

Looks fine to me, thank you for tracking down on all the little things to make it work!

(Minor optional comments below.)

.github/workflows/gh-ci-cron.yaml

orbeckst · 2023-01-02T20:44:43Z

.github/workflows/gh-ci-cron.yaml

+      # overwrite installs by picking up nightly wheels
+    - name: nightly_wheels
+      run: |
+        pip install --pre -U -i https://pypi.anaconda.org/scipy-wheels-nightly/simple scipy numpy h5py matplotlib


comment: there's a lot of .\\agic hidden in these action files... I would have no idea where to look for these wheels.

I think one of the next major tasks here will be to a) document the CI actions, b) clean them up (it's grown a lot beyond what I initially planned back at the start of last year when I squashed things into using re-usable actions).

Just to add here before I forget - there is a draft SPEC for nightly wheels: https://scientific-python.org/specs/spec-0004/

It needs a lot of fleshing out though...

orbeckst · 2023-01-02T20:45:40Z

.github/workflows/gh-ci-cron.yaml

+        pip install --pre -U -i https://pypi.anaconda.org/scipy-wheels-nightly/simple scipy numpy h5py matplotlib
+
+
+    - name: check_deps


probably more accurately named "list deps"

orbeckst · 2023-01-02T20:47:52Z

package/MDAnalysis/visualization/streamlines_3D.py

@@ -281,15 +281,15 @@ def update_dictionary_point_in_cube_start_frame(array_simulation_particle_coordi
                                                           axis=0)
                cube['centroid_of_particles_first_frame'] = centroid_particles_in_cube
            else:  # empty cube
-                cube['centroid_of_particles_first_frame'] = 'empty'
+                cube['centroid_of_particles_first_frame'] = None


apparently not covered by tests... not sure if this is incorrect or if we must or can cover it but I'll mention it

O that is a bit problematic, we really must cover this. It also means we're getting 'empty' entries being fed somewhere else 🙀 Could you place a blocking review here?

aaaaahhh I was wondering why I was getting to print in that branch but CI wasn't picking up forced errors.. we're using multiprocessing 🤦🏽

I'll have a quick look around but it looks like:
a) Errors aren't being propagated from child workers (hence why it didn't raise the relevant "truth of an array" error),
b) Codecov isn't picking up on coverage since it's not being properly logged by the child workers... we do have the multiprocessing thing set in coveragerc, but it's obviously not working

orbeckst · 2023-01-02T20:48:21Z

package/MDAnalysis/visualization/streamlines_3D.py

            cube_counter += 1

    def update_dictionary_end_frame(array_simulation_particle_coordinates, dictionary_cube_data_this_core):
        """Update the cube dictionary objects again as appropriate for the second and final frame."""
        cube_counter = 0
        for key, cube in dictionary_cube_data_this_core.items():
            # if there were no particles in the cube in the first frame, then set dx,dy,dz each to 0
-            if cube['centroid_of_particles_first_frame'] == 'empty':
+            if cube['centroid_of_particles_first_frame'] is None:


comment: Looked innocently enough...

I'm surprised it's the only failure we have! 1.25 is going to cause a lot of trouble for legacy downstream codes.

there's a really similar 'empty' check in

mdanalysis/package/MDAnalysis/visualization/streamlines.py

Line 204 in 0c4f823

if square_1_centroid == 'empty' or square_2_centroid == 'empty':

Those probably also need to get changed...

Can confirm the same FutureWarning is happening in streamlines :/ the test isn't good enough to pick it up though.

orbeckst

Test assignment of None instead of "empty", please.

IAlibay · 2023-01-03T22:24:02Z

I'm a little bit confused as to why this specific branch doesn't get covered, it's definitely being entered by some of the workers, and both coverage.py and pytest-cov claim that it should work...

I wonder if there's a bad termination happening somewhere else.

IAlibay · 2023-01-04T01:01:16Z

.github/workflows/gh-ci-cron.yaml

@@ -64,7 +76,7 @@ jobs:

    - name: run_tests
      run: |
-        pytest -n $numprocs testsuite/MDAnalysisTests --disable-pytest-warnings --durations=50
+        pytest -n $numprocs testsuite/MDAnalysisTests --durations=50 -W error::FutureWarning


Ideally we'd error on DeprecationWarnings too, but we emit too many of them internally for this to be viable. At least this way we can maybe pick up potential issues?

IAlibay · 2023-01-04T01:02:20Z

package/MDAnalysis/visualization/streamlines.py

@@ -176,7 +176,7 @@ def produce_list_centroids_this_frame(list_indices_in_polygon):
        list_centroids_this_frame = []
        for indices in list_indices_in_polygon:
            if not indices[0].size > 0:  # if there are no particles of interest in this particular square
-                list_centroids_this_frame.append('empty')


Same issue as per streamlines_3D, also can't get coverage over it (even though the new test assigns None to everything (hence the zeroed array return)

IAlibay · 2023-01-04T14:06:00Z

Ok @orbeckst I think I'm at a point here where codecov is just not playing ball and there's not much else I can think of / find in the docs / issues. I don't understand the problem enough to be able to do a proper bug report either.

Short story is that those code branches are being covered, but they aren't being reported for some reason or another.

My suggestion at this point is to go with this knowing the above, and not delay 2.4.2 any further.

orbeckst

Thanks for all the digging.

Perhaps add CHANGELOG entry for the internal fix in streamlines and streamlines2D? At a later point in time, this will be an actual error so I think it qualifies for a mentioning — it's a fix for a future error that would have happened.

IAlibay · 2023-01-04T15:49:48Z

Perhaps add CHANGELOG entry for the internal fix in streamlines and streamlines2D? At a later point in time, this will be an actual error so I think it qualifies for a mentioning — it's a fix for a future error that would have happened.

I might not be following what you mean here @orbeckst, do you mean something else than what has already been added to the 2.4.2 entry for these?

orbeckst · 2023-01-04T15:52:06Z

Ah, sorry — the CHANGELOG checkmark was missing in the top comment (EDIT: I checked it now) and the first file entry was not CHANGELOG (as I was used to) so I wrongly assumed that there was no CHANGELOG entry. My bad. The one you have is perfect.

IAlibay · 2023-01-04T18:00:17Z

Ok now to cherry pick everything into a 2.4.2 release.

* Fix cron jobs against nightly wheels * Address incompatibilities with NumPy 1.25 changes in streamlines and streamlines_3d

check what's installed by cron job

4bc48d5

IAlibay added 2 commits December 20, 2022 09:50

add workflow trigger

0a75811

move away from mamba since it won't respect pip installs

0b1f58f

IAlibay changed the title ~~[WIP] Investigate why cron job isn't not pick up nightly NumPy changes~~ [WIP] Investigate why cron job isn't picking up nightly NumPy changes Dec 20, 2022

IAlibay added 2 commits December 20, 2022 11:02

add build dependencies

ca2c07a

Merge branch 'develop' into investigate-cron

4324868

IAlibay mentioned this pull request Dec 27, 2022

[maint] Bugfix release 2.4.2 #3980

Closed

2 tasks

IAlibay added 5 commits December 27, 2022 18:05

strict channel priority for cron ci

8b1e8bf

switch things around

a6feb4c

commas around your 3.10

4cd3458

upgrade on the pip install

58249dc

use pre flag for pip

2895585

IAlibay added 3 commits December 27, 2022 23:43

check if it's caused by numpy 1.25

f239d37

add numpy nightly wheels back in

4f82253

circumvent elementwise comparison completely

1ad1508

IAlibay commented Jan 2, 2023

View reviewed changes

IAlibay changed the title ~~[WIP] Investigate why cron job isn't picking up nightly NumPy changes~~ Investigate why cron job isn't picking up nightly NumPy changes Jan 2, 2023

IAlibay added 3 commits January 2, 2023 18:58

is not ==

e2a2da0

Add changelog entry, make cron-ci cron-only

4ccd37c

tie off 2.4.2 release

3dc9773

IAlibay requested review from tylerjereddy and orbeckst January 2, 2023 19:19

orbeckst approved these changes Jan 2, 2023

View reviewed changes

orbeckst requested changes Jan 2, 2023

View reviewed changes

IAlibay added 2 commits January 2, 2023 21:15

Is actions CI different?

3724efe

minor changes to CI yaml file

b5b521c

IAlibay added 3 commits January 2, 2023 21:39

try directly invoking it from the commandline

3b3e396

remove intentional ValueError raising

10f9ae1

try using the sigterm variable

ff70268

JecaTosovic mentioned this pull request Jan 3, 2023

Update numpy requirement from <1.24 to <1.25 JecaTosovic/WaterNetworkAnalysis#4

Closed

Maybe the try/finally encapsulation does some magic?

deb7499

IAlibay added 5 commits January 3, 2023 22:42

try just one entry in coveragerc

301a359

add streamlines 2D fix

d3c4130

get pytest to fail on FutureWarning for cron CI

2a008a3

temporarily undo cron only trigger for action

9d96661

does reverting coveragerc changes affect codecov report?

a16d035

IAlibay commented Jan 4, 2023

View reviewed changes

IAlibay changed the title ~~Investigate why cron job isn't picking up nightly NumPy changes~~ Fix nightly NumPy cron job, add numpy 1.25-dev0 fixes Jan 4, 2023

IAlibay added 3 commits January 4, 2023 14:01

Update changelog, remove try branch in streamlines

2bc1f88

update changelog date for 2.4.2

c5dab47

set cron job to only work via schedule

81e2891

orbeckst approved these changes Jan 4, 2023

View reviewed changes

correct 1.25 not 1.24

2f6f577

IAlibay merged commit 8d43073 into develop Jan 4, 2023

IAlibay deleted the investigate-cron branch January 4, 2023 17:59

IAlibay added a commit that referenced this pull request Jan 4, 2023

Fix nightly NumPy cron job, add numpy 1.25-dev0 fixes (#3977)

75b0337

* Fix cron jobs against nightly wheels * Address incompatibilities with NumPy 1.25 changes in streamlines and streamlines_3d

IAlibay mentioned this pull request Jan 4, 2023

pre-release 2.4.2 #3987

Merged

2 tasks

IAlibay added the maintainability label Sep 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix nightly NumPy cron job, add numpy 1.25-dev0 fixes #3977

Fix nightly NumPy cron job, add numpy 1.25-dev0 fixes #3977

IAlibay commented Dec 20, 2022 •

edited by orbeckst

Loading

codecov bot commented Dec 20, 2022 •

edited

Loading

IAlibay commented Dec 20, 2022

IAlibay commented Dec 27, 2022 •

edited

Loading

IAlibay Jan 2, 2023

IAlibay Jan 2, 2023

IAlibay commented Jan 2, 2023

orbeckst left a comment

orbeckst Jan 2, 2023

IAlibay Jan 2, 2023

IAlibay Jan 3, 2023

orbeckst Jan 2, 2023

orbeckst Jan 2, 2023

IAlibay Jan 2, 2023

orbeckst Jan 2, 2023

IAlibay Jan 2, 2023 •

edited

Loading

orbeckst Jan 2, 2023

IAlibay Jan 2, 2023

IAlibay Jan 2, 2023

IAlibay Jan 3, 2023

orbeckst left a comment

IAlibay commented Jan 3, 2023

IAlibay Jan 4, 2023

IAlibay Jan 4, 2023

IAlibay commented Jan 4, 2023 •

edited

Loading

orbeckst left a comment

IAlibay commented Jan 4, 2023

orbeckst commented Jan 4, 2023 •

edited

Loading

IAlibay commented Jan 4, 2023

		pip install --pre -U -i https://pypi.anaconda.org/scipy-wheels-nightly/simple scipy numpy h5py matplotlib


		- name: check_deps

Fix nightly NumPy cron job, add numpy 1.25-dev0 fixes #3977

Fix nightly NumPy cron job, add numpy 1.25-dev0 fixes #3977

Conversation

IAlibay commented Dec 20, 2022 • edited by orbeckst Loading

PR Checklist

codecov bot commented Dec 20, 2022 • edited Loading

Codecov Report

IAlibay commented Dec 20, 2022

IAlibay commented Dec 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IAlibay commented Jan 2, 2023

orbeckst 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 Jan 2, 2023 • edited Loading

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

orbeckst left a comment

Choose a reason for hiding this comment

IAlibay commented Jan 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IAlibay commented Jan 4, 2023 • edited Loading

orbeckst left a comment

Choose a reason for hiding this comment

IAlibay commented Jan 4, 2023

orbeckst commented Jan 4, 2023 • edited Loading

IAlibay commented Jan 4, 2023

IAlibay commented Dec 20, 2022 •

edited by orbeckst

Loading

codecov bot commented Dec 20, 2022 •

edited

Loading

IAlibay commented Dec 27, 2022 •

edited

Loading

IAlibay Jan 2, 2023 •

edited

Loading

IAlibay commented Jan 4, 2023 •

edited

Loading

orbeckst commented Jan 4, 2023 •

edited

Loading