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

fix llvmlite version for python 2 #327

Merged
merged 2 commits into from
Feb 9, 2021
Merged

fix llvmlite version for python 2 #327

merged 2 commits into from
Feb 9, 2021

Conversation

adriaanph
Copy link
Member

llvmlite 0.32.0 was the last to support python 2, but the default dependency check looked for >=0.31.0dev0

llvmlite 0.32.0 was the last to support python 2, but the default dependency check looked for >=0.31.0dev0
@adriaanph
Copy link
Member Author

@ludwigschwardt please review - github doesn't allow me to add reviewers to this PR.

@ludwigschwardt
Copy link
Contributor

ludwigschwardt commented Feb 8, 2021

This could be called a Numba bug (see e.g. numba/numba#5035). Whether there is appetite to backport the max_llvmlite_version of numba/numba#5046 to 0.47.0 and cut a 0.47.1 release is unclear, but that would be the cleanest solution.

It could even be a pip bug, since pip is picking a newer llvmlite source package from the local cache, even though it is incompatible with the Python version. At least that's what happens on my machine (and I suspect on yours too).

setup.py Outdated
@@ -61,7 +61,7 @@
install_requires=['numpy >= 1.12.0', 'katpoint >= 0.9', 'h5py >= 2.3', 'numba',
'katsdptelstate[rdb] >= 0.8', 'dask[array] >= 1.2.1',
'requests >= 2.18.0', 'pyjwt', 'future',
'cityhash >= 0.2.2'],
'cityhash >= 0.2.2', 'llvmlite==0.32.0'],
Copy link
Contributor

Choose a reason for hiding this comment

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

A more appropriate change would be:

'llvmlite < 0.32; python_version < "3"'

llvmlite 0.32.0 is already too late, since it is Python 3 only. You want 0.31.0 (and any patch releases on top of that if it ever happens).

Copy link
Member Author

Choose a reason for hiding this comment

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

awesome, thanks for background & the proposed refinement.

@ludwigschwardt
Copy link
Contributor

ludwigschwardt commented Feb 9, 2021

@bmerry, any opinion on this? Should we go ahead and fix this in katdal, or get the Numba guys to cut a 0.47.1 release? 😄

@bmerry
Copy link
Contributor

bmerry commented Feb 9, 2021

It looks like the problem is that llvmlite didn't set python_requires in 0.32. Unfortunately that can't be fixed after the fact; even if there is a 0.32.1 release that has it, that'll just stop pip picking up 0.32.1, but won't stop it picking up 0.32.

My opinionated opinion is that we shouldn't be spending any effort on supporting Python 2 and that people who insist on using it have to solve their own problems, but if you are going to do something, this seems like the simplest solution.

@ludwigschwardt
Copy link
Contributor

Thanks, I haven't even looked at llvmlite as the culprit 🙂

This discussion also points out this issue and references PEP 592. It looks like there is now a way to "yank" a released file from PyPI - good to remember.

@ludwigschwardt ludwigschwardt merged commit 6e30a7a into ska-sa:python2 Feb 9, 2021
@adriaanph
Copy link
Member Author

thanks, gentlemen.

this change does not have zero value - it is critical for continued support of katholog, which is currently still stuck in python2. it would be wasteful to discard any useful work that is generated within the organisation.

@adriaanph adriaanph deleted the patch-1 branch February 10, 2021 11:02
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.

None yet

3 participants