Refactor manylinux tag logic #1
Conversation
There was a problem hiding this comment.
This is definitely a lot clearer and easier to follow. A few minor nits but otherwise this is definitely better than the quick hack I had written. Thanks for making this so much better. Let me know what you think about the comments. Happy to make the modifications as well and merge it into this branch.
conda_lock/pypi_solver.py
Outdated
| @@ -39,6 +42,7 @@ | |||
| if TYPE_CHECKING: | |||
| from packaging.tags import Tag | |||
|
|
|||
| logger = logging.getLogger(__name__) | |||
| @@ -55,6 +59,12 @@ class PlatformEnv(Env): | |||
| Fake poetry Env to match PyPI distributions to the target conda environment | |||
| """ | |||
|
|
|||
| _sys_platform: Literal["darwin", "linux", "win32"] | |||
There was a problem hiding this comment.
This is a type annotation, so if you're running a type checker like mypy or pyright it will know what are the valid values of _sys_platform. I find it really useful because if you're in another file and you hover over a variable it'll show you the type and hence the possible values, making it much easier to figure out what's going on without needing to infer it from reading the code.
So in this case I was reading the code, I figured out that these are the possible values, and I declared them as such. And now the type checker statically verifies this and can offer hints.
| elif len(matches) == 1: | ||
| return matches[0] | ||
| else: | ||
| lowest = min(matches) |
There was a problem hiding this comment.
this will technically not work for those "special cases" because Version("2010") > Version("2.27") I suspect. I don't think this is a case that is very common though so maybe we can ignore it or error out? Or special case the special cases like you do below.
There was a problem hiding this comment.
According to my understanding, there is no glibc v2010, so this should never happen. I made an attempt to explain this in the docstring.
There was a problem hiding this comment.
Ah yes, you are right, 2010 was actually the old manylinux name for 2.12 so yes, the glibc version would be 2.12.
| # is most preferred so we reverse the order. | ||
| compatible_manylinux_tags = [ | ||
| tag | ||
| for tag in reversed(MANYLINUX_TAGS) |
There was a problem hiding this comment.
small nit: there is a tad of a wasted thing here because once one matches, all the rest will match. I don't think it has a huge consequence though and this code is definitely easier to follow.
There was a problem hiding this comment.
You mean that we could break the iteration early? Technically yes, but that's more of a nanosecond optimization, right?
|
I really appreciate the very thorough review! I think I've addressed all your points. Feel free to squash-merge if you think it's ready. |
romain-intel
merged commit 48c8a69
into
romain-intel:fix/respect-glibc-version
* Refactor manylinux tag logic * Run doctests * Remove unused logger * Clarify that legacy manylinux tags aren't glibc versions
I was struggling to follow the previous logic, so I read the manylinux PEPs and decided to refactor.
I wanted to make sure we were covering all possible edge cases.
Also, I agree that using
packaging.version.Versionis overkill here, but throughoutconda-lockwe're parsing versions all over the place, and so things get a bit crazy if locally we're constantly reimplementing string-parsing logic. And indeed, I uncovered that the previous commit was mixing_and.separators:Does this refactor look clear and correct? (Feel free to skip looking at the new test I wrote; it's pretty tedious.) Thanks!!