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

Rebuild for python310 (redux) #175

Merged
merged 14 commits into from
Nov 13, 2021
Merged

Conversation

h-vetinari
Copy link
Member

@h-vetinari h-vetinari commented Nov 9, 2021

Based on #174, but cleaning up the left over test skips, invocation & CPU settings from drone/travis.

Closes #174

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@h-vetinari
Copy link
Member Author

PPC looks like it's going to be an utter mess of failures again (presumably the same kind of QEMU bugs that are responsible for the scipy test suite failing on PPC-via-QEMU; scipy is currently built without running the test suite, as people were opposed to dropping it completely, and preferred untested binaries to nothing, see discussion in conda-forge/scipy-feedstock#191).

An excerpt:

...........FF.......s...EEEEEEEEEEEEE..EEEE............................s [  6%]
sssssssssssssssssssssssssssss.....................................FFF... [  6%]
......F....................FFFF.....F...FFF...FFF..F..........F.F..FFFFF [  7%]
..F...F..F........................................F.....FF.........FF... [  7%]
......FFFFFF..FFFFFFFFFF...FFFFFFFFFFF.............F......F..........F.. [  7%]
......F.........F.....F..........F...........F..FF.F..F..F..F...F.F.FF.F [  8%]
F.FF.FF.F.FFFF.F..FFFFF....................................F............ [  8%]
..F............F.F............FF...........F..FEEEFEEEFEEEFEEFEEE..FEEE. [  8%]
FF..F.FF..F.......F........F.F......................ssssssssssssssssFsss [  9%]
sssssssssss...........F.......F......F.....F..F.........F.FFF.FFF.FFF... [  9%]
...........F..F.....FFFFFF.F...FF...FF..............F...F.F.FF..F.F.FFFF [  9%]
FFF.F...FFF..FFF.F..FFFF...F....F....FF..FF..FFFFF...FF..FF......FF..... [ 10%]
........FF.F....F..F.........................F.......................F.F [ 10%]
F.F...FF.F.FFF.FF...F....FF..................F.........FFFFFFF..FFFFFFFF [ 10%]
FF...FFFFFFFFFFFFFFFFFF.........F.F.....FFFFF.FFF.FFF.F........FF.FF.... [ 11%]
.........FFFF........................................................... [ 11%]
.................F.F........x.....................................FF.FF. [ 11%]
FF.FF.....F......F..FF....FF...F...FF......................FF..F........ [ 12%]
............F..F..F..........F.........F.F..FFFFFF............F...FFFFF. [ 12%]
.........F...F......FsF...F.........F......F.....F.....F......F..FF.FFF. [ 12%]
FF...F....F...F....F.................................................... [ 13%]

@glemaitre
Copy link
Member

So conda-forge switched all the builds to Azure?

@adrinjalali
Copy link
Member

I'd be okay with untested binary for ppc

@glemaitre
Copy link
Member

I'd be okay with untested binary for PPC

+1 on my side for this release build

@h-vetinari
Copy link
Member Author

So conda-forge switched all the builds to Azure?

Involuntarily. Both Travis and Drone became more or less unusable over the last 1-2 months.

@h-vetinari
Copy link
Member Author

I'd be okay with untested binary for ppc

The following is hardly a picture of confidence (~1450 failures+errors), but I'm happy to go along with the more pragmatic approach here.

= 1319 failed, 18656 passed, 1445 skipped, 60 xfailed, 39 xpassed, 3359 warnings, 133 errors in 13833.57s (3:50:33) =

@h-vetinari
Copy link
Member Author

@isuruf @jakirkham
Just some visibility that - per this PR - the test suite is not being run for PPC anymore. Otherwise there's about 1450 failures + errors. This is how I interpreted the discussion regarding dropping PPC on the scipy-feedstock, but decisions might be different case-by-case?

there hasn't yet been a pypy run completed in QEMU on azure,
so we take test timings of a cpython run that hit a slower
agent (all added tests ran >80 sec there)
@h-vetinari
Copy link
Member Author

@conda-forge/scikit-learn
In addition to the point about PPC, this is now skipping a bunch of tests on aarch (& especially pypy), but at least it's green. 😅
It's possible that aarch+pypy only passes on the faster azure agents, but that's nothing an occasional restart can't fix (and this PR is about cpython anyway).

@adrinjalali
Copy link
Member

yeah those are quite slow. Thanks for skipping them.

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

I think this makes sense.

recipe/meta.yaml Outdated Show resolved Hide resolved
recipe/meta.yaml Outdated Show resolved Hide resolved
@h-vetinari
Copy link
Member Author

win + pypy "only" ran into numpy.core._exceptions._ArrayMemoryError: Unable to allocate <x> MiB - I think this can be considered spurious (considering that it passed before).

If desired, I can try something like setting the pagefile size explicitly (c.f. e.g. conda-forge/scipy-feedstock@fcf4f63) - not sure if that'll help though, as the error is usually more explicitly about running out of pagefile.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipe) and found some lint.

Here's what I've got...

For recipe:

  • Failed to even lint the recipe, probably because of a conda-smithy bug 😢. This likely indicates a problem in your meta.yaml, though. To get a traceback to help figure out what's going on, install conda-smithy and run conda smithy recipe-lint . from the recipe directory.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@h-vetinari
Copy link
Member Author

Someone feel like pressing the merge button? 🙃

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

Minor comment otherwise LGTM. Thanks for working on this @h-vetinari !

- export OMP_NUM_THREADS=1 # [aarch64 or ppc64le]
- pytest --pyargs sklearn -k "not ({{ tests_to_skip }})" -n {{ cpus }} --durations=50 {{ extra_pytest_args }}
- pytest --pyargs sklearn -k "not ({{ tests_to_skip }})" -nauto --timeout=1200 --durations=50 # [not ppc64le]
Copy link
Member

@rth rth Nov 11, 2021

Choose a reason for hiding this comment

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

The nauto is only reliable when psutils is installed. e.g. when running inside docker with limited CPU resources https://github.com/pytest-dev/pytest-xdist/blob/2bac85562b996059b152b3491d73ddbbc413318a/src/xdist/plugin.py#L30 is this the case in the CI? Otherwise it would often return say 16 CPU while CI can only use 2.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, interesting point. On azure this seems to work reliably, i.e. there are only ever 2-3 workers per image/run. It's IMO an improvement over explicitly setting a number of processors (as previously), which might then not all be available. Though even in that case, the CI passed, just slower due to contention.

@h-vetinari
Copy link
Member Author

Last commit is just a comment update; I believe this PR is ready for an automerge-label 🙃

@rth rth added the automerge Merge the PR when CI passes label Nov 12, 2021
@rth
Copy link
Member

rth commented Nov 12, 2021

I believe this PR is ready for an automerge-label upside_down_face

It's done. Thanks again for your work @h-vetinari !

@github-actions
Copy link
Contributor

github-actions bot commented Nov 13, 2021

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

  • linter: passed
  • azure: passed

Thus the PR was passing and merged! Have a great day!

@h-vetinari
Copy link
Member Author

The win+pypy failure is spurious:

E       numpy.core._exceptions._ArrayMemoryError: Unable to allocate 22.9 MiB for an array with shape (300, 5000) and data type complex128

Can someone please press merge? I can also join as a maintainer if people want - that way I wouldn't have to ping as often 😋

@h-vetinari h-vetinari closed this Nov 13, 2021
@h-vetinari h-vetinari reopened this Nov 13, 2021
@github-actions github-actions bot merged commit d674415 into conda-forge:master Nov 13, 2021
@h-vetinari h-vetinari deleted the 3.10 branch November 13, 2021 07:57
@xhochy xhochy mentioned this pull request Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge the PR when CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants