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

[WIP] Build POT against oldest-supported-numpy #347

Closed
wants to merge 5 commits into from
Closed

[WIP] Build POT against oldest-supported-numpy #347

wants to merge 5 commits into from

Conversation

davidghiurco
Copy link
Contributor

@davidghiurco davidghiurco commented Feb 11, 2022

Types of changes

Build / setup:

  • Compiling against the oldest possible numpy version possible given the platform, while also still allowing numpy>=1.20 to be used
    ** NOTE: I suspect POT would happily support & compile against even older numpy versions, and maintain forward compatibility with newer numpy, but I'm not sure.
  • Removing legacy code workarounds to build-time dependency problems which have been already fixed by the inclusion of a pyproject.toml in [WIP] POT build without installing cython first #293
  • Set python_requires to >=3.7. Python 3.6 support was dropped in POT release 0.8.1. This configuration is also necessary to help oldest-supported-numpy properly determine what the oldest version of numpy it should use during setup.

Motivation and context / Related issue

Address #346

How has this been tested (if it applies)

PR checklist

  • I have read the CONTRIBUTING document.
  • The documentation is up-to-date with the changes I made (check build artifacts).
  • All tests passed, and additional code has been covered with new tests.
  • I have added the PR and Issue fix to the RELEASES.md file.

@davidghiurco
Copy link
Contributor Author

davidghiurco commented Feb 11, 2022

@rflamary Given these changes, we might be able to roll-back the numpy requirement bump made in #326. If POT doesn't use anything special (at build time) from numpy==1.20.0, we could allow oldest-numpy-version to compile against even older versions (by decreasing the minimum Python version requirement in python_requires of setup.py, which, in turn, would allow older numpy versions).

The reason I introduced the minimum Python version / python_requires>=3.7 in the first place, is to respect the already merged precedent that Python 3.6 support is dropped.

But it looks like it might have been dropped pre-maturely due to the nature of the numpy>=1.20 build-time constraint that is now improved in this PR. I realize that a Python 3.6 runtime can still use POT by compiling it from source, but the proper way to specify what versions of Python you support is by using python_requires. I'd be happy to bump python_requires down to >=3.6 and re-enable wheel building for cp36, now that wheels will be properly built against the proper numpy version. Let me know what you think.

setup.py Outdated Show resolved Hide resolved
@davidghiurco
Copy link
Contributor Author

davidghiurco commented Feb 11, 2022

If you change numpy>=1.20 -> numpy>=1.16 in install_requires, you should also change python_requires=">=3.7" -> python_requires=">=3.6", because numpy<1.20 is not compatible with Python 3.7+ anyways.

That is if you want to re-add Python 3.6 support.

setup.py Outdated Show resolved Hide resolved
@rflamary
Copy link
Collaborator

Hello @davidghiurco and thank you for the PR. I'm not sure why but the github actions for building the wheels are not triggered so I will probably ope a new PR wfrom this one on a branc of POT to ensure that the wheels are building OK before merging

@davidghiurco
Copy link
Contributor Author

davidghiurco commented Feb 15, 2022

@rflamary
Copy link
Collaborator

well build all wheels is longer because it build on ARM for linux but both should run, i really believe i need to have the pr on a branch of POT to make it work (but i don't see why it does not run in PR only from the github action conditions...)

@rflamary
Copy link
Collaborator

I'm closing this PR since #349 have now been merged, thanks again @davidghiurco we will have a smooth release next time ;)

@rflamary rflamary closed this Feb 23, 2022
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.

2 participants