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

Migrate tests to pytest #244

Merged
merged 5 commits into from
Nov 18, 2024
Merged

Conversation

emilazy
Copy link
Contributor

@emilazy emilazy commented Oct 11, 2024

Hi there. The nose Python test framework has been deprecated and unmaintained for over a decade, and doesn’t work out of the box on Python 3.12. Over in NixOS we’ve been working to remove it and migrating packages as we go. I’ve done an automatic migration to the modern pytest framework here with the nose2pytest tool, and cleaned up the remaining references manually.

Unfortunately and ironically, I can’t adequately test this PR as the mhcflurry package is broken in NixOS for unrelated TensorFlow reasons right now, but I hope what I have here can be useful and if you run into any issues testing it I’m happy to help fix them :) Thanks for your work on such an incredible project!

@emilazy
Copy link
Contributor Author

emilazy commented Oct 11, 2024

Apologies; as expected my untested changes are broken. If you’re interested in the migration I can try to fix it up, even though I don’t have an easy way to test it end‐to‐end right now.

@@ -13,7 +13,6 @@

from mhcflurry import Class1AffinityPredictor

from nose.tools import eq_, assert_raises
Copy link
Contributor

Choose a reason for hiding this comment

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

The CI is failing because you've removed assert_raises here, yet the function is still being called in this test file.

@ndalchau
Copy link
Contributor

I think this is a nice addition, as it will help maintain this version of MHCflurry into the future. But it's up to @timodonnell
to approve :)

It should be an easy fix to get the CI running (see my comment above). What is the challenge behind having an end-to-end test for this repo? I didn't have a problem running this locally.

@emilazy
Copy link
Contributor Author

emilazy commented Oct 27, 2024

Apologies for the untested PR :) nose2pytest is somewhat unsophisticated so it missed a few things and mangled some syntax. I’ve pushed fixes for the ones reported by CI; hopefully it will be happy now.

The reason I sent this without testing is that the MHCflurry package in Nixpkgs is currently broken due to our out‐of‐date TensorFlow, and we don’t currently support TensorFlow on macOS, which meant I couldn’t get a proper build environment going to test this. I use Nixpkgs for all my package management, so it’d be somewhat awkward to install a bunch of stuff outside of that for a codebase I was doing drive‐by fixes for. I realize that playing CI whack‐a‐mole when I can’t easily directly test my changes isn’t great, though… just didn’t want to leave my branch rotting on my disk in case it could be helpful to you.

@emilazy
Copy link
Contributor Author

emilazy commented Nov 4, 2024

I’m not sure what’s up with the latest test failures, but from what I can see they don’t relate to the nose → pytest conversion. Maybe a NumPy version compatibility issue or something?

@ndalchau
Copy link
Contributor

ndalchau commented Nov 5, 2024

I just took a look on my system. You're quite right that there is a problem with numpy. The last successful run of the CI brought down numpy version 1.26.4: https://github.com/openvax/mhcflurry/actions/runs/11137285619.
In this PR, the CI brought down version 2.0.2. I'm pretty sure this is the problem. We need to raise a new issue for this.

@emilazy
Copy link
Contributor Author

emilazy commented Nov 7, 2024

Ah, yes – a lot of packages aren’t ready for NumPy 2 yet, so you’re in good company :) There’s a migration guide you might find helpful, but the simplest solution for now is probably to add an upper version bound to whichever of your dependencies is pulling NumPy in so that CI works again. Happy to rebase this PR if that’s done so that the changes to the test suite can be verified.

@ndalchau
Copy link
Contributor

ndalchau commented Nov 8, 2024

Can you try rebasing please?

@emilazy
Copy link
Contributor Author

emilazy commented Nov 8, 2024

Done :) Thanks for your patience with this!

@ndalchau
Copy link
Contributor

@timodonnell , the CI is awaiting approval

@emilazy
Copy link
Contributor Author

emilazy commented Nov 13, 2024

Looks like the conversion tool couldn’t figure out when it was numpy.testing.assert_equal instead of nose’s. Hopefully this should fix the last few failing tests!

@ndalchau
Copy link
Contributor

Looks good to me. Over to the maintainers now.

@timodonnell
Copy link
Contributor

Thanks @emilazy and @ndalchau for all of your work on this! Looks great to me. Very much appreciated.

@timodonnell timodonnell merged commit 8e9f353 into openvax:master Nov 18, 2024
3 checks passed
@emilazy emilazy deleted the push-qonlzoyrmkms branch November 18, 2024 20:39
@GaetanLepage
Copy link

Indeed, thanks all for this great patch that should help us fixing mhcflurry in nixpkgs.
@timodonnell do you plan to have a release (that would contain this patch) any time soon ?
If not we will vendor the patch ourselves in the meantime.

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.

4 participants