-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New resolver breaks with mmcv_full package and custom find-links #9183
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
Comments
FWIW, that version number looks invalid, I suspect the new resolver only works with PEP 440 compliant versions. |
(Maybe it should support non compliant versions as the old resolver does, but just looking at the code there, it's using internal APIs that only work on PEP 440 valid versions) |
The invalid version in question came from a wheel. Wheel only allows PEP 440 versions. |
I don't think that's historically true, the Wheel PEP predates PEP 440. |
To be clear, I'm perfectly OK with the idea that the new resolver mandates PEP 440 versions, since they're the only versions we actually know how to interpret correctly, and as a general case of ratcheting to be stricter in what sort of malformed, or non standards based data we accept. |
So, I suppose my question is how should mmcv change such that it is possible to specify which wheel you need depending on your torch and cuda (or lack there of) versions? Is this possible with PEP 440 version, a quick look seems like it might not be? In that case what can mmcv do? |
What mmcv is trying to do isn't exactly well supported by the tooling, so it's going to be a bit of a square peg round hole problem. That being said, they could fix the synatical problems with their PEP 440 version by doing something like That doesn't solve the larger problem about how to handle this case, but it would fix the problem with local versions being an invalid syntax. |
You’re correct, the PEP 440 is not added until Metadata 1.2, and wheels are allowed to use 1.1. I think it’s the easiest way out to just allow legacy versions here. (It would only result in a build failure later in this case, however, since mmcv wheels are actually using Metadata 2.1, making them invalid). |
If we do decide to allow legacy versions (I'm in the "let's be strict and make people fix their metadata" camp, so if I'm making the call -- no changes beyond a better error message needed here), I'd like to gate allowing legacy versions to only wheels that declare to be under metadata versions < 1.2. Everything over that, should still get the exact same failure because PEP 440 is a thing and what pip saw is an invalid wheel. |
I'm also inclined to be stricter. Metadata 1.2 mandates that versions follow PEP 440, so we can definitely be strict with anything that claims to be 1.2 or later. Personally, I'm also fine with saying we only support PEP 440, even for older versions, because honestly, 1.2 is 15 years old by now, I think we can afford to not carry legacy code that's solely there to handle packages that are still using standards that are 15 years out of date. |
Yup. I'm on board with not supporting software standards from back when I was 5. :) enjoys the moment, as everyone around me feels old |
I think it's fine to mandate PEP 440, I do think we should maybe try to get a better error message for it though. |
Looks like we are all tending toward this! :) |
That's good enough for me. I'm happy to accept the answer mmcv should fix this on their end. |
For anyone reading this who doesn't know how to temporarily switch to old resolver behavior while using pip 20.3.x: use the flag |
I'm not sure if this is an mmcv issue or a pip issue. I've posted a corresponding issue with mmcv here: open-mmlab/mmcv#679
What did you want to do?
MMCV contains custom binaries targeted towards specific versions of pytorch and custom cuda or cpu configurations. Previously with the old resolver you specified a target version you wanted as such:
When working on pip 20.3 and master I get this error:
Is there a different way that the version
'1.0.5+torch1.6.0+cpu'
should be specified? Or is this something that the new dependency resolver should be able to handle, but it doesn't?Disabling the new resolver fixes the issue, but that's a unstable workaround as the old resolver will soon be removed.
The text was updated successfully, but these errors were encountered: