Test and CI modernization #370
Conversation
|
Note: conda env caching in CI is enabled using the bz2 requirement. This is going to create problems down the line as fewer packages are being distributed in bz2 format now (in favor of .conda). This is a known issue of the setup-miniconda action, but does not yet appear to be resolved. |
|
It looks like our minimum test environment is proving difficult to solve. I suspect this is because of the hard pin to scipy 1.0.0. @craffel what do you think about bumping this to 1.2.0 to be contemporaneous with the new minimum numpy (1.14)? I expect in 2024 nobody will gripe about having to upgrade from scipy 1.0 to 1.2. If we bump to 1.4 instead, it might afford us the opportunity to update some of the bsseval code to use scipy.fft instead of the older fftpack API. (I'll leave that as a separate, totally optional issue.) (EDIT: had my versions wrong) |
Seems totally fine to me. |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
|
Ugh. I had to bump numpy to 1.15.4 (nov 2018) and matplotlib 3.3.0 (jul 2020) to get the minimal env working consistently, but I think this is fine. We were already bumping numpy to 1.14, and I doubt anyone will be left out by going to 15 instead of 14. |
|
Alright, we're all green finally. I think this should be good to merge. Test coverage is generally good, excepting the cases that I explicitly skipped out. We won't have codecov action status here until things merge into the main branch, but you can check it here: https://app.codecov.io/gh/craffel/mir_eval/tree/bmcfee%2Fmir_eval%3Amodernization/mir_eval Alignment has some spotty coverage that could be improved with more defensive tests, but I think that's out of scope for this PR (which is already monstrous). In terms of changes to the main code base, it's primarily spelling corrections, docstring corrections (eg wrong variable names caught by velin), and formatting via black. The tests/ folder has been basically rewritten, but the core logic of the tests is essentially unchanged. Everything else has to do with package metadata and github workflow configuration. |
|
Any objection to merging? |
There was a problem hiding this comment.
Some minor questions - I skipped reviewing the library changes because I'm assuming they are all from black (perhaps in retrospect we should have had a separate PR which was just the result of running black, and then have this PR for introducing the modernization, but oh well).
| environment-file: ${{ matrix.envfile }} | ||
| # Disabling bz2 to get more recent dependencies. | ||
| # NOTE: this breaks cache support, so CI will be slower. | ||
| use-only-tar-bz2: False # IMPORTANT: This needs to be set for caching to work properly! |
There was a problem hiding this comment.
Important for this to be set, but it's not set... I think we can remove this comment given your above comments.
There was a problem hiding this comment.
The comment comes from the starter code for the action. I think it's serving a useful purpose here to indicate that caching does not currently work, and it cannot until the setup-miniconda action gets fixed upstream. I'd prefer to leave it in as a provenance trail for why the flag is set the way it is.
| @@ -60,7 +70,22 @@ jobs: | |||
| shell: bash -l {0} | |||
| run: python -c "import numpy; numpy.show_config()" | |||
|
|
|||
| - name: Show libraries in the system on which SciPy was built | |||
| shell: bash -l {0} | |||
| run: python -c "import scipy; scipy.show_config()" | |||
There was a problem hiding this comment.
Is this something you were using for debugging, or do we want it always?
There was a problem hiding this comment.
I think it's good to include. Scipy configs can change out from under us as dependencies update, and these things have historically caused us trouble (eg transcription velocity recently, bsseval a few years back).
| name = mir_eval | ||
| version = attr: mir_eval.__version__ | ||
| description = Common metrics for common audio/music processing tasks. | ||
| author = Colin Raffel |
There was a problem hiding this comment.
Can we just change this to "mir_eval developers" now or something?
There was a problem hiding this comment.
Sure. Should also update in the sphinx configs, but it's your copyright there so I'll leave it to you to propose a change here. 😁
There was a problem hiding this comment.
... we can also deal with that as a separate PR when we update for the migration to an mir_eval org.
Yeah, though I did want to add linting / spellchecking / doc checking as CI actions here, which weren't going to pass until we did this anyway. But yes, I can confirm there are no functional changes to the library code. |
|
Cool, LGTM! |
|
... and thank you for this monumental effort. |
This PR will implement a slew of updates to bring us into 2024:
My goal here isn't necessarily to improve any functionality. However, it's possible that the updates above in the test suite may uncover failures that need addressing.
fixes #269
fixes #331
fixes #352