Skip to content

TEST: Only use stable argsorts in PARREC tests #1234

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

Merged
merged 2 commits into from
Jun 27, 2023
Merged

TEST: Only use stable argsorts in PARREC tests #1234

merged 2 commits into from
Jun 27, 2023

Conversation

effigies
Copy link
Member

@effigies effigies commented Jun 26, 2023

numpy/numpy#23707 introduced an optimized argsort that will be used on some platforms, including GitHub actions' runners, making this a nasty issue to figure out. I'm very glad that it wasn't on some obscure architecture that one user hit, as I have no idea how I would have debugged that.


Between the last good and first bad runs, numpy released 1.25 and GitHub Actions released new Python images. First starting by verifying that the previous versions of both resolve the issue. Then will relax numpy.

@codecov
Copy link

codecov bot commented Jun 26, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (b7022e0) 92.16% compared to head (c385a53) 92.16%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1234   +/-   ##
=======================================
  Coverage   92.16%   92.16%           
=======================================
  Files          98       98           
  Lines       12364    12364           
  Branches     2539     2539           
=======================================
  Hits        11395    11395           
  Misses        646      646           
  Partials      323      323           

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

@effigies
Copy link
Member Author

Okay. So it is Numpy 1.25. Still unclear why it's 100% of the time here and I've been unable to reproduce even stochastically on my local machine.

@effigies
Copy link
Member Author

Looking through the commit logs between the last succeeding and first failing pre-releases: numpy/numpy@126b46c...v1.25.0

I think numpy/numpy#23707 is a likely candidate. Will try finding alternatives to argsort for the failing tests.

@effigies effigies changed the title DEBUG: Identify source of CI failures TEST: Remove (potentially) unstable argsorts from PARREC tests Jun 27, 2023
@effigies effigies marked this pull request as ready for review June 27, 2023 13:29
@effigies
Copy link
Member Author

effigies commented Jun 27, 2023

@grlee77 Would you mind having a look? Does this seem like a thing worth reporting upstream, or is instability in argsort considered to be acceptable? (Nevermind. I'll go ahead and report and they can close it if they like.)

@effigies
Copy link
Member Author

Bug report here: numpy/numpy#24064

We'll see if we need to include this patch or if we should just tolerate failures until numpy releases a fix.

@effigies effigies changed the title TEST: Remove (potentially) unstable argsorts from PARREC tests TEST: Only use stable argsorts in PARREC tests Jun 27, 2023
@effigies
Copy link
Member Author

Nevermind. This was our fault for not explicitly requesting a stable argsort. Merging.

@effigies effigies merged commit 064e80d into master Jun 27, 2023
@effigies effigies deleted the debug branch June 27, 2023 15:10
effigies added a commit that referenced this pull request Jul 13, 2023
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.

1 participant