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

CI: test with Python 3.12-beta #18689

Merged
merged 4 commits into from Jul 21, 2023
Merged

CI: test with Python 3.12-beta #18689

merged 4 commits into from Jul 21, 2023

Conversation

tupui
Copy link
Member

@tupui tupui commented Jun 16, 2023

Python 3.12 beta2 is out today. Probably a good time to add it in our CI.

Closes #18922

[skip cirrus] [skip circle]
@tupui tupui added the CI Items related to the CI tools such as CircleCI, GitHub Actions or Azure label Jun 16, 2023
@tupui tupui added this to the 1.12.0 milestone Jun 16, 2023
@tupui
Copy link
Member Author

tupui commented Jun 16, 2023

haha we can't yet: ModuleNotFoundError: No module named 'distutils'

@tupui tupui marked this pull request as draft June 16, 2023 13:47
@rgommers
Copy link
Member

I'm afraid that there is no point doing this until there is a numpy nightly for 3.12. That may take a few more weeks (or, with luck, it lands after this weekend, dunno).

@EwoutH
Copy link
Contributor

EwoutH commented Jun 19, 2023

NumPy wheels are incoming: numpy/numpy#23991

@rgommers
Copy link
Member

@tupui numpy nightlies are up now. If you change this to use 3.12-dev in the CI job that uses the scientific-python bucket rather than the one you currently modified, it should work.

@tupui
Copy link
Member Author

tupui commented Jun 20, 2023

We can still use main. Do we want the wheel instead?

I relaunched and now the error is on our side after successfully having compiled NumPy:

Traceback (most recent call last):
  File "/home/runner/work/scipy/scipy/dev.py", line 119, in <module>
    from distutils import dist
ModuleNotFoundError: No module named 'distutils'

And indeed we have even wrote something about it (gh-16058):

scipy/dev.py

Lines 114 to 120 in f05674a

# distutils is required to infer meson install path
# if this needs to be replaced for Python 3.12 support and there's no
# stdlib alternative, use CmdAction and the hack discussed in gh-16058
with warnings.catch_warnings():
warnings.filterwarnings("ignore", category=DeprecationWarning)
from distutils import dist
from distutils.command.install import INSTALL_SCHEMES

@FFY00 seems like you were involved in some discussions around that topic. Any guidance on the best course of action as of now? Thanks in advance 😃


Another question that was raised on other repos... Should we also try to ship wheels for 3.12? Seems like CirrusCI is ready: https://cibuildwheel.readthedocs.io/en/stable/options/#build-skip

@rgommers
Copy link
Member

Another question that was raised on other repos... Should we also try to ship wheels for 3.12? Seems like CirrusCI is ready: https://cibuildwheel.readthedocs.io/en/stable/options/#build-skip

Yes, we can start building nightlies for 3.12-beta I'd say, that will help other projects like scikit-learn & co. Probably best as a follow-up PR to this one. The GHA and Cirrus CI changes should be the same as in numpy/numpy#23991 I'd expect.

@FFY00
Copy link
Contributor

FFY00 commented Jun 27, 2023

@FFY00 seems like you were involved in some discussions around that topic. Any guidance on the best course of action as of now? Thanks in advance 😃

Sorry for the delay! I wasn't able to look at this earlier, but I am finally catching up with my notifications.

For Python 3.12 and forward, Debian seems to now be patching sysconfig. The main thing to keep in mind is that their patching is also present inside virtual environments, so you need to make sure you only use platlib from deb_system when you aren't in a virtual environment, or similar. This can be achieved by looking at sysconfig.get_default_scheme(), which will return posix_local when you are on the global environment.

So, you'd change

scipy/dev.py

Lines 335 to 348 in f05674a

def get_site_packages(self):
"""
Depending on whether we have debian python or not,
return dist_packages path or site_packages path.
"""
if 'deb_system' in INSTALL_SCHEMES:
# debian patched python in use
install_cmd = dist.Distribution().get_command_obj('install')
install_cmd.select_scheme('deb_system')
install_cmd.finalize_options()
plat_path = Path(install_cmd.install_platlib)
else:
plat_path = Path(get_path('platlib'))
return self.installed / plat_path.relative_to(sys.exec_prefix)

to

def get_site_packages(self):
    """
    Depending on whether we have debian python or not,
    return dist_packages path or site_packages path.
    """
    if sys.version_info < (3, 12) and 'deb_system' in INSTALL_SCHEMES:
        # debian patched python in use
        install_cmd = dist.Distribution().get_command_obj('install')
        install_cmd.select_scheme('deb_system')
        install_cmd.finalize_options()
        plat_path = Path(install_cmd.install_platlib)
    elif sysconfig.get_default_scheme() == 'posix_prefix':
        # debian patched python in use, and we are on the global environment
        # posix_prefix is the user-installable debian scheme
        plat_path = Path(get_path('platlib', scheme='deb_system'))
    else:
        plat_path = Path(get_path('platlib'))
    return self.installed / plat_path.relative_to(sys.exec_prefix)

However, I'd actually respect Debian's patching and install to posix_local unless you have a reason not to. I don't know the motivation behind doing install_cmd.select_scheme('deb_system') in your current code, but I suspect it might have just been copied from somewhere else.

The adjusted code:

def get_site_packages(self):
    """
    Depending on whether we have debian python or not,
    return dist_packages path or site_packages path.
    """
    if sys.version_info < (3, 12) and 'deb_system' in INSTALL_SCHEMES:
        # debian patched python in use
        install_cmd = dist.Distribution().get_command_obj('install')
        install_cmd.finalize_options()
        plat_path = Path(install_cmd.install_platlib)
    else:
        plat_path = Path(get_path('platlib'))
    return self.installed / plat_path.relative_to(sys.exec_prefix)

No special handling is needed for Debian anymore because they now patch sysconfig.

That is, unless you actually want to install to the Debian install directory (/usr/lib/python3.12/dist-packages), which is usually reserved for Debian packages, instead of the Debian-adjusted user install directory (/usr/local/lib/python3.12/dist-packages).

@FFY00
Copy link
Contributor

FFY00 commented Jun 27, 2023

It seems this code was introduced in 9e19577. @AnirudhDagar, do you remember if there was a specific reason behind installing to deb_system, or if it was just inherited from the code it was based on?

@AnirudhDagar
Copy link
Member

Thanks @FFY00 for sharing the update on Debian Python 3.12 sysconfig patch. It was there in the first place because we wanted to mimic the exact behaviour of mesonbuild for installation to avoid path inconsistencies. IIRC, meson was using deb_system.

No special handling is needed for Debian anymore because they now patch sysconfig.

That is, unless you actually want to install to the Debian install directory (/usr/lib/python3.12/dist-packages), which is usually reserved for Debian packages, instead of the Debian-adjusted user install directory (/usr/local/lib/python3.12/dist-packages).

This makes sense, we don't need to specifically install to Debian install directory, as long as we make sure the path behaviour is the same as mesonbuild's.

@tupui
Copy link
Member Author

tupui commented Jun 28, 2023

@AnirudhDagar would you like to handle this?

@AnirudhDagar
Copy link
Member

Thanks, happy to take this up. @tupui is it okay if I send a PR early next week?
I'm occupied with a lot of office work this week.

@tupui
Copy link
Member Author

tupui commented Jun 29, 2023

Sure thank you @AnirudhDagar 🙏

@andyfaff
Copy link
Contributor

Reopened this PR. #18924 will enable this PR to go further in CI for cp312, it wasn't a replacement for this PR.

@tupui tupui marked this pull request as ready for review July 21, 2023 01:09
Copy link
Member

@AnirudhDagar AnirudhDagar left a comment

Choose a reason for hiding this comment

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

As mentioned, this PR will require installing setuptools in the 3.12 CI job since meson has not gotten rid of distutils yet (open PR mesonbuild/meson#11133)

.github/workflows/linux_meson.yml Outdated Show resolved Hide resolved
Co-authored-by: Anirudh Dagar <anirudhdagar6@gmail.com>
@andyfaff
Copy link
Contributor

This is looking good. I think the test fail is unrelated?

@tupui
Copy link
Member Author

tupui commented Jul 21, 2023

Yes seems unrelated, thanks both! Let's get this in

@tupui tupui merged commit 404d3df into scipy:main Jul 21, 2023
24 of 25 checks passed
@lesteve
Copy link
Contributor

lesteve commented Jul 21, 2023

Great timing, for scikit-learn I was interested to add Python 3.12 testing and I found this PR that just got merged!

It would be great to have nightly wheels uploaded as mentioned in #18689 (comment).

I am willing to take inspiration from numpy and have a go at a scipy PR, but if someone from Scipy plans to do it, it may be quicker, let me know!

@andyfaff
Copy link
Contributor

I'm trying to build nightlies at https://github.com/andyfaff/scipy/actions/runs/5619957108/job/15227994408, but running into issues building numpy within the cibuildwheel context.

@rgommers
Copy link
Member

It seems like cibuildwheel has no dedicated option to use another wheelhouse, nor do I see an option to use a pre-release from PyPI. So even when numpy uploads a pre-release wheel in ~10 days when Python 3.12.0rc1 arrives, it needs a custom install I think. Using CIBW_BEFORE_BUILD is the way to go I think, for nightlies compatible with numpy nightlies we anyway need to use those numpy nightlies, and not rebuild numpy 1.25.x. So I think putting this in CIBW_BEFORE_BUILD is the way to go:

pip install -i https://pypi.anaconda.org/scientific-python-nightly-wheels/simple numpy

@tupui tupui deleted the latest_python_dev branch July 24, 2023 09:10
tylerjereddy pushed a commit to tylerjereddy/scipy that referenced this pull request Sep 26, 2023
Fix scipygh-18689 scipygh-18922

python3.12 removes distutils and scipy dev.py uses
distutils for the meson based build in debian python.
With python3.12 debian has patched sysconfig and no
longer requires dependence on distutils.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Items related to the CI tools such as CircleCI, GitHub Actions or Azure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: dev.py has distutils usage
7 participants