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

Declare Python 3.13 as supported in sagelib #39188

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

@antonio-rojas
Copy link
Contributor Author

antonio-rojas commented Dec 22, 2024

Needs ipython/ipykernel@b47db6f applied to ipykernel to fix some test failures due to deprecation warnings. Not sure if it's worth to filter out the warnings in Sage

Copy link

github-actions bot commented Dec 22, 2024

Documentation preview for this PR (built with commit 335b6dd; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@tornaria
Copy link
Contributor

Nice, thanks for working this out. It's very useful. I'm currently working on updating sagemath to 10.5 on python 3.13, I'll report here when I'm done.

Copy link
Contributor

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

Could you please also make similar changes to the pyproject.toml in the root? (I can do this also separately if you prefer.)

@tornaria
Copy link
Contributor

I'm testing with sagemath 10.5 + the 6 PRs listed here, and I'm getting some test failures when running tests in parallel which seem to indicate some concurrency issues.

For instance:

$ sage -tp 16 src/sage/misc/
[...]
sage -t --warn-long 10.0 --random-seed=89600877682973310340044898742861036918 sage/misc/session.pyx
**********************************************************************
File "sage/misc/session.pyx", line 293, in sage.misc.session.save_session
Failed example:
    g = cython_lambda('double x', 'x*x + 1.5')
Exception raised:
    Traceback (most recent call last):
      File "/builddir/sagemath-10.5/.xbps-testdir/1734922373/usr/lib/python3.13/site-packages/sage/doctest/forker.py", line 718, in _run 
        self.compile_and_execute(example, compiler, test.globs)
        ~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/builddir/sagemath-10.5/.xbps-testdir/1734922373/usr/lib/python3.13/site-packages/sage/doctest/forker.py", line 1142, in compile_and_execute
        exec(compiled, globs)
        ~~~~^^^^^^^^^^^^^^^^^
      File "<doctest sage.misc.session.save_session[9]>", line 1, in <module>
        g = cython_lambda('double x', 'x*x + 1.5')
      File "sage/misc/lazy_import.pyx", line 406, in sage.misc.lazy_import.LazyImport.__call__ (build/cythonized/sage/misc/lazy_import.c:6293)
        return self.get_object()(*args, **kwds)
      File "/builddir/sagemath-10.5/.xbps-testdir/1734922373/usr/lib/python3.13/site-packages/sage/misc/cython.py", line 531, in cython_lambda
        cython_import_all(tmpfile, d, verbose=verbose, **kwds)
        ~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/builddir/sagemath-10.5/.xbps-testdir/1734922373/usr/lib/python3.13/site-packages/sage/misc/cython.py", line 582, in cython_import_all
        m = cython_import(filename, **kwds)
      File "/builddir/sagemath-10.5/.xbps-testdir/1734922373/usr/lib/python3.13/site-packages/sage/misc/cython.py", line 553, in cython_import
        name, build_dir = cython(filename, **kwds)
                          ~~~~~~^^^^^^^^^^^^^^^^^^
      File "/builddir/sagemath-10.5/.xbps-testdir/1734922373/usr/lib/python3.13/site-packages/sage/misc/cython.py", line 356, in cython
        with restore_cwd(target_dir):
             ~~~~~~~~~~~^^^^^^^^^^^^
      File "/usr/lib/python3.13/contextlib.py", line 141, in __enter__
        return next(self.gen)
      File "sage/misc/sage_ostools.pyx", line 76, in restore_cwd (build/cythonized/sage/misc/sage_ostools.c:3524)
        os.chdir(chdir)
    FileNotFoundError: [Errno 2] No such file or directory: '/tmp/tmpuqiw_uhw/spyx/_tmp_tmpqj1d6rz9_tmp_awc6bqij_pyx'
**********************************************************************
[...]

Has anyone seen this?

I think what happens is:

  • the way caching works in sage.misc.temporary_file.spyx_tmp() means every doctest runner will use the same temporary directory (here /tmp/tmpuqiw_uhw/spyx).
  • for some reason, the atexit callback is run before this test finishes, and the whole directory and all its contents are wiped.

If I run the whole testsuite in non-parallel mode, everything seems to work fine (then I'm just left with a couple of unrelated failures).

If I disable the atexit.register(...) call, this also seems to fix all the issues.

There is this restore_atexit() magic, maybe that was affected with python 3.13?

@tornaria
Copy link
Contributor

Two questions on procedure:

  • should this PR include the dependencies here so the branch actually works?
  • does it make sense to have a PR for python 3.13 support that is based on 10.5?

@antonio-rojas
Copy link
Contributor Author

I think what happens is:

  • the way caching works in sage.misc.temporary_file.spyx_tmp() means every doctest runner will use the same temporary directory (here /tmp/tmpuqiw_uhw/spyx).
  • for some reason, the atexit callback is run before this test finishes, and the whole directory and all its contents are wiped.

Right, atexit is now run in all child processes. See python/cpython#114279 for discussion

@tornaria
Copy link
Contributor

Right, atexit is now run in all child processes. See python/cpython#114279 for discussion

Ok, the new behaviour of python 3.13 for some reason causes the TemporaryDirectory() in sage.misc.temporary_file.spyx_tmp() to be deleted on child exit.

One workaround is to use TemporaryDirectory(delete=False) but according to docs this is only available starting with python 3.12. I'm just using tmp_dir() instead which doesn't seem to have the same issue.

I'll prepare a draft PR but here are some questions for @orlitzky:

  • Is there a reason to use TemporaryDirectory() instead of tmp_dir() ?
  • For the TMP_DIR_FILENAME_BASE, is it really necessary to cleanup from an atexit hook? It doesn't seem to be necessary here (I only tested on python 3.12 and 3.13).
  • Any reason not to use prefix='sage_' for that temporary directory?

This is what I have in mind:

--- a/src/sage/misc/temporary_file.py
+++ b/src/sage/misc/temporary_file.py
@@ -540,7 +540,5 @@ def spyx_tmp() -> str:
     if _spyx_tmp:
         return _spyx_tmp
 
-    d = tempfile.TemporaryDirectory()
-    _spyx_tmp = os.path.join(d.name, 'spyx')
-    atexit.register(lambda: d.cleanup())
+    _spyx_tmp = tmp_dir(name='spyx_')
     return _spyx_tmp

tornaria added a commit to tornaria/sage that referenced this pull request Dec 24, 2024
For some reason, a new behaviour of python 3.13 [0] causes the
`TemporaryDirectory()` in `sage.misc.temporary_file.spyx_tmp()`
to be deleted on child exit, which causes trouble with parallel
doctesting [1].

We rewrite `spyx_tmp()` using `tmp_dir()`, which doesn't have this
problem, see [2].

[0] python/cpython#114279
[1] sagemath#39188 (comment)
[2] sagemath#39188 (comment)
@tornaria tornaria mentioned this pull request Dec 24, 2024
3 tasks
@orlitzky
Copy link
Contributor

I'll prepare a draft PR but here are some questions for @orlitzky:

  • Is there a reason to use TemporaryDirectory() instead of tmp_dir() ?

I'm trying (infrequently) to get rid of tmp_dir() and tmp_filename() because they leave junk around for longer than they need to -- they only clean up via the atexit hook on TMP_DIR_FILENAME_BASE, which only happens on a clean exit. A few users have reported problems from /tmp filling up. It's also pretty pointless to have a function in sagelib that does almost exactly what a python standard library function does, but worse.

But for spyx_tmp? Not really. The way it is used, the spyx_tmp() directory lives for as long as sage does anyway. The spyx_tmp directory has all of these same problems but I've ignored it so far because I don't think we should be trying to ship a cython compiler in a math library in the first place.

  • For the TMP_DIR_FILENAME_BASE, is it really necessary to cleanup from an atexit hook? It doesn't seem to be necessary here (I only tested on python 3.12 and 3.13).

Otherwise it may not be cleaned up at all? sage-cleaner might do it too (I don't remember) but if you are calling sage functions from python code you won't have sage-cleaner.

  • Any reason not to use prefix='sage_' for that temporary directory?

No.

@tornaria
Copy link
Contributor

I'll prepare a draft PR but here are some questions for @orlitzky:

  • Is there a reason to use TemporaryDirectory() instead of tmp_dir() ?

I'm trying (infrequently) to get rid of tmp_dir() and tmp_filename() because they leave junk around for longer than they need to -- they only clean up via the atexit hook on TMP_DIR_FILENAME_BASE, which only happens on a clean exit. A few users have reported problems from /tmp filling up. It's also pretty pointless to have a function in sagelib that does almost exactly what a python standard library function does, but worse.

Makes sense. It still seems nice to have a "parent" temporary directory where everything is placed, and easier to clean up at once if everything else fails. But sure, for many purposes the temporary files should be short lived.

But for spyx_tmp? Not really. The way it is used, the spyx_tmp() directory lives for as long as sage does anyway. The spyx_tmp directory has all of these same problems but I've ignored it so far because I don't think we should be trying to ship a cython compiler in a math library in the first place.

Ok, this is what is broken with python 3.13 and (hopefully) fixed by #39201. The issue seems to be that the TemporaryDirectory as a local variable to the spyx_tmp() function is destroyed on child exit. I suppose fork() makes a clone of this variable in a separate memory space, so destroying the clone does not affect the original variable except for side-effects. For some reason this doesn't happen with a TemporaryDirectory on a global variable... (for python < 3.13 exit from a child will not call the exit functions)

  • For the TMP_DIR_FILENAME_BASE, is it really necessary to cleanup from an atexit hook? It doesn't seem to be necessary here (I only tested on python 3.12 and 3.13).

Otherwise it may not be cleaned up at all? sage-cleaner might do it too (I don't remember) but if you are calling sage functions from python code you won't have sage-cleaner.

But python itself will clean it up on exit. That's what the manual claims, and that's what I see. I'm sure there are exceptional circumstances that will skip this clean up, but most likely those exceptional circumstances will also skip the atexit handler.

In any case, I guess there's no harm on keeping the atexit handler, except in ths sense that having it there gives the false impression that the directory will not be cleaned up otherwise.

  • Any reason not to use prefix='sage_' for that temporary directory?

No.

Will do then.

tornaria added a commit to tornaria/sage that referenced this pull request Dec 24, 2024
For some reason, a new behaviour of python 3.13 [0] causes the
`TemporaryDirectory()` in `sage.misc.temporary_file.spyx_tmp()`
to be deleted on child exit, which causes trouble with parallel
doctesting [1].

We rewrite `spyx_tmp()` using `tmp_dir()`, which doesn't have this
problem, see [2].

[0] python/cpython#114279
[1] sagemath#39188 (comment)
[2] sagemath#39188 (comment)
@orlitzky
Copy link
Contributor

orlitzky commented Dec 24, 2024

But python itself will clean it up on exit. That's what the manual claims, and that's what I see. I'm sure there are exceptional circumstances that will skip this clean up, but most likely those exceptional circumstances will also skip the atexit handler.

You are right, but I didn't know it at the time. What the documentation actually says is,

On completion of the context or destruction of the temporary directory object the newly created temporary directory and all its contents are removed from the filesystem.

Surely python objects are destroyed when the process exits, right? Actually, no:

It is not guaranteed that del() methods are called for objects that still exist when the interpreter exits. weakref.finalize provides a straightforward way to register a cleanup function to be called when an object is garbage collected.

However, at least in python-3.12, TemporaryDirectory() does use weakref.finalize(), so it is guaranteed to clean up. You have to read the source to find out though, and apparently I did that on October 23, 2023, long after I made the change to TMP_DIR_FILENAME_BASE.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants