Opened 9 months ago

Closed 3 months ago

Last modified 6 days ago

#33213 closed enhancement (fixed)

Replace SAGE_TMP by the system location in the sage library

Reported by: Michael Orlitzky Owned by:
Priority: minor Milestone: sage-9.7
Component: misc Keywords:
Cc: Samuel Lelièvre, Travis Scrimshaw, François Bissey Merged in:
Authors: Michael Orlitzky Reviewers: Matthias Koeppe, Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: d94a1e6 (Commits, GitHub, GitLab) Commit:
Dependencies: #33829, #33790 Stopgaps:

Status badges

Description (last modified by Michael Orlitzky)

Re: https://groups.google.com/g/sage-devel/c/zhjl_j6j_Qc

These days, using a SAGE_TMP that by default lives under $HOME is overly complex and often inefficient. In this ticket we implement the first phase of its removal, to be replaced by python's tempfile module. Specifically,

  1. We replace all direct uses of SAGE_TMP within the sage library and doctests.
  2. We update tmp_dir() and tmp_filename() to use the tempfile defaults.
  3. We remove SAGE_TMP.

Afterward, the custom functions tmp_dir() and tmp_filename() can be deprecated in favor of tempfile.TemporaryDirectory() and tempfile.NamedTemporaryFile().

Moreover when #8784 is done, we'll be able to remove sage-cleaner entirely.

Change History (97)

comment:1 Changed 8 months ago by Michael Orlitzky

Branch: u/mjo/ticket/33213
Commit: ffd9aa6a082cc6195efc1977a1d9f23b8d73a91e
Description: modified (diff)
Summary: Avoid SAGE_TMP for truly transient files and directoriesReplace SAGE_TMP by the system location in the sage library

I've posted a work-in-progress branch that raises a question. The python docs,

https://docs.python.org/3/library/tempfile.html

state that, in regards to a named temporary file,

Whether the name can be used to open the file a second time, while the named temporary file is still open, varies across platforms (it can be so used on Unix; it cannot on Windows NT or later)

So in many cases, where I would have liked to have done

sage: with tempfile.NamedTemporaryFile() as f:
....:     q.save_image(f.name)

I have instead done something like

sage: f = tempfile.NamedTemporaryFile(delete=False); f.close()
sage: q.save_image(f.name)
sage: os.remove(f.name)

or

sage: with tempfile.TemporaryDirectory() as d:
....:     filename = os.path.join(d, "foo.png") 
....:     q.save_image(filename)

to avoid the save_image call reopening the same name. However, the docs are unclear about what "on Windows" means. So the two examples above can be simplified if this is not a problem on cygwin.


Last 10 new commits:

dabb1c7Trac #33213: replace SAGE_TMP in repl doctests.
c292ea8Trac #33213: replace SAGE_TMP in magma interface doctests.
172e9c4Trac #33213: replace SAGE_TMP in dist script doctests.
24499f1Trac #33213: remove mentions of SAGE_TMP from the lazy_string module.
3c8c0e6Trac #33213: replace SAGE_TMP in session doctests.
3db8444Trac #33213: remove SAGE_TMP from tmp_dir() and tmp_filename().
5e7e080Trac #33213: don't clear SAGE_TMP upon exiting.
ea2fefdTrac #33213: remove SAGE_TMP from sage.misc.all.
b880571Trac #33213: replace SAGE_TMP in cleaner/quit interfaces.
ffd9aa6Trac #33213: drop SAGE_TMP from sage.misc.misc.

comment:2 Changed 8 months ago by git

Commit: ffd9aa6a082cc6195efc1977a1d9f23b8d73a91ef2b85e2dd38fd78ff300ec31da462c946332568a

Branch pushed to git repo; I updated commit sha1. New commits:

03941c1Trac #33213: don't reset expect interfaces when starting them.
339ac94Trac #33213: simplify GAP workspace doctest.
f2b85e2Trac #33213: close an open file a bit sooner in gap interface.

comment:3 Changed 8 months ago by git

Commit: f2b85e2dd38fd78ff300ec31da462c946332568a0f90d98f5782a25d10b5ae1797953673cb7dc468

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

976d88bTrac #33213: remove SAGE_TMP from tmp_dir() and tmp_filename().
8e09502Trac #33213: don't clear SAGE_TMP upon exiting.
a22fa71Trac #33213: remove SAGE_TMP from sage.misc.all.
810c855Trac #33213: replace SAGE_TMP in cleaner/quit interfaces.
ce5a76dTrac #33213: drop SAGE_TMP from sage.misc.misc.
955fdbaTrac #33213: remove the sage-cleaner program.
a9df953Trac #33213: remove "remote cleaner" option from expect interface.
9f504b5Trac #33213: remove sage.interfaces.cleaner.
92e84daTrac #33213: close an open file a bit sooner in gap interface.
0f90d98Trac #33213: use @cached_function for the spawned processes file.

comment:4 Changed 8 months ago by Michael Orlitzky

Dependencies: #8784
Description: modified (diff)

Making #8784 a dependency because we can avoid many contortions on this branch if we don't have to keep sage-cleaner aware of where the spawned_processes file lives.

comment:5 Changed 8 months ago by William Stein

How did you solve the problems that sage-cleaner solves? It's not like I wrote it for no reason. There are many situations in practice that people hit where temp files and subprocess get left around. Make sure that at least the following works:

  1. Start sage. Do something that creates a temp file and also something that spawns a subprocess.
  2. Segfault the sage that you started, e.g., by doing something stupid with Cython.

What happens? Are all temp files and spawned processes properly cleaned up?

It's an unfortunate reality that we make Cython relatively easy to use, hence it's easy from the command line or notebook to segfault a running Sage process.

Just curious. I never liked or wanted to write sage-cleaner, but I couldn't think of any better approach to make end users happy.

comment:6 in reply to:  5 Changed 8 months ago by Michael Orlitzky

Replying to was:

How did you solve the problems that sage-cleaner solves? It's not like I wrote it for no reason. There are many situations in practice that people hit where temp files and subprocess get left around. Make sure that at least the following works:

  1. Start sage. Do something that creates a temp file and also something that spawns a subprocess.
  2. Segfault the sage that you started, e.g., by doing something stupid with Cython.

What happens? Are all temp files and spawned processes properly cleaned up?

  1. Now that all temporary files are created in /tmp or equivalent, they'll be cleaned up by the system eventually. In the event of a crash, I would say that this is preferable, because those temporary files can contain important debugging information.
  1. Sage doesn't segfault. More seriously, it depends on the process. In the simplest case, the subprocess will continue doing whatever it's doing until it completes, and will then terminate. Next would be subprocesses that communicate with the parent; eventually they'll be sent a SIGPIPE and commit suicide. If you go out of your way to launch something that is basically a daemon, though, it can indeed outlive the parent.

For (2) to be an issue, you have to launch a standalone process that never terminates, detach it from the parent, compile a C extension, load the extension into your current session, and then cause it to segfault. I don't think cleaning up after that situation should be the responsibility of any application; but especially not of a computer algebra system.

If you intend to do something like that frequently (or host a service for users who will...), there are better solutions these days -- cgroups on linux being the best example. Keeping in mind that sage-cleaner works by parsing a spawned_processes file that must be updated every time a process is launched, solving the problem at the OS level in particular has the advantage that it will catch sub-subprocesses. Sage-cleaner generally cannot (unless the subprocess is also sage), because the subprocess doesn't know that it's supposed to add a line item to spawned_processes.

comment:7 Changed 8 months ago by Michael Orlitzky

I should also point out that it's not critical that we remove sage-cleaner at this point. The main hurdle wrt the removal of SAGE_TMP is that sage-cleaner and the sage library (register_spawned_process()) need to agree on a location for the spawned processes file. The current branch uses a temporary file in the library that lives for the duration of the sage session, and that's of no use to sage-cleaner due to the random name. We could still store the spawned processes file with a fixed name in DOT_SAGE, though, probably with the current sage PID tacked onto the path somewhere.

I saw this as a good time to remove it because its one crucial job -- to clean up SAGE_TMP under $HOME -- is now obsolete.

comment:8 Changed 8 months ago by git

Commit: 0f90d98f5782a25d10b5ae1797953673cb7dc468de4a00ee85328833e7ce679a7c86be9aee3e489f

Branch pushed to git repo; I updated commit sha1. New commits:

de4a00eTrac #33213: ensure temporary gap workspace is created in a doctest.

comment:9 Changed 8 months ago by Michael Orlitzky

Status: newneeds_review

I believe I've fixed all of the test failures so I'm setting this to needs_review to see what the patchbots think.

comment:11 Changed 8 months ago by William Stein

Thanks @mjo for your explanation. Your responses don't necessarily apply in the context of users that got me to write the sage-cleaner in the first place, and they will probably be pretty annoyed if they ever realize what happened. Sage is different than some more less complicated computer algebra software like Magma, because Sage much more frequently spawns subprocesses. Fortunately, over the years the number of subprocesses Sage spawns has been reduced due to writing and using C library interfaces instead of pexpect, etc.

Regarding /tmp, unless you specifically set something up to periodically clean up files, unless things have changed a lot, don't most Linux distros *not* automatically delete files from there, possibly until a reboot? You're making a huge change from "temp files get cleaned up by Sage itself quickly" to "maybe someday the system will clean them up". Maybe at this point that is the right change, but it's important to acknowledge that it is a significant change.

I agree that when users are using Linux, have sufficient privileges to use containers, and also actually know how to use them, then these same problems can be solved in a more robust way. However, this is a small percentage of users of Sage. Many users use other operating systems or don't have admin permissions.

CoCalc? is the main platform for running Sage that I care about these days, and every individual "project" is its own container, /tmp is extremely ephemeral, etc., so everything you say applies there. That said, users of CoCalc? can't use Docker or other container mechanisms themselves within projects, due to security constraints.

In any case, I'm not personally worried about these changes having any adverse impact for me. However, I'm leaving this comment here so people who are impacted can maybe know they may have to revert these patches for their own install of Sage. Probably the way things will go is that this gets merged, and a year later certain environments start finding that Sage is leaving around clutter in a way that Sage didn't for the last decade, but users don't know why. Hopefully they google and find this comment.

comment:12 in reply to:  11 Changed 8 months ago by Michael Orlitzky

Replying to was:

Regarding /tmp, unless you specifically set something up to periodically clean up files, unless things have changed a lot, don't most Linux distros *not* automatically delete files from there, possibly until a reboot? You're making a huge change from "temp files get cleaned up by Sage itself quickly" to "maybe someday the system will clean them up". Maybe at this point that is the right change, but it's important to acknowledge that it is a significant change.... Probably the way things will go is that this gets merged, and a year later certain environments start finding that Sage is leaving around clutter in a way that Sage didn't for the last decade, but users don't know why. Hopefully they google and find this comment.

Systemd comes with a timer (like a cron job) to clean up /tmp, but I'm not familiar with its out-of-the-box configuration on mainstream distros. I still use OpenRC myself, where a reboot is what triggers it.

On systems like mine, "leftover junk" is a fair criticism, but keep in mind the bigger picture: the python tempfile.TemporaryDirectory() and tempfile.NamedTemporaryFile() functions automatically remove the file/directory as soon as it's done being used. In this branch, I've changed a bunch of manual SAGE_TMP code to use those two functions, so a lot of what would have been leftover junk is now simply not created in the first place. Any remaining junk is due to sage's own tmp_filename() and tmp_dir(), which expect you to clean up the file yourself, or assume that sage-cleaner will do it eventually.

If this gets merged, we can deprecate tmp_filename() and tmp_dir() as obsolete in favor of the python functions. Over a hopefully-short amount of time, that should eliminate the leftover junk at the source.

comment:13 Changed 8 months ago by Dima Pasechnik

indeed, I am not at all worried about junk left at /tmp, as, at least as far as documentation of Python is correct, the facility of NamedTemporaryFile cleans up after itself automatically (I guess it uses POSIX's tmpfile(3) behind the scene).

I am a bit puzzled by the need to manually keep them open using delete=False the call to NamedTemporaryFile. Is it because one wants to keep them for the duration of a Sage session?

comment:14 in reply to:  13 ; Changed 8 months ago by Michael Orlitzky

Replying to dimpase:

I am a bit puzzled by the need to manually keep them open using delete=False the call to NamedTemporaryFile. Is it because one wants to keep them for the duration of a Sage session?

If you're referring to my new code (in many doctests), then this is simply a precaution for cygwin (see comment:1). I would love to simplify it if someone with a cygwin installation says that the "on Windows" warning doesn't apply there.

comment:15 in reply to:  14 Changed 8 months ago by Michael Orlitzky

Replying to mjo:

If you're referring to my new code (in many doctests), then this is simply a precaution for cygwin (see comment:1). I would love to simplify it if someone with a cygwin installation says that the "on Windows" warning doesn't apply there.

(If it DOES apply on cygwin, we could improve this in the future by updating the functions that now take a filename to also accept an open file handle; that will avoid re-open()ing it.)

comment:16 Changed 8 months ago by Dima Pasechnik

Cygwin is POSIX emulator, so on Windows doesn't really apply to it.

comment:17 in reply to:  16 Changed 8 months ago by Michael Orlitzky

Replying to dimpase:

Cygwin is POSIX emulator, so on Windows doesn't really apply to it.

You have two wishes remaining =)

I'm "happy" to go back and change all of those doctests, but I'd like confirmation before I spend half a day doing it.

comment:18 Changed 8 months ago by Dima Pasechnik

Cc: Samuel Lelièvre added

Perhaps slelievre can test this on Cygwin - if you specify in some detail what to look for.

comment:19 in reply to:  18 Changed 8 months ago by Michael Orlitzky

Replying to dimpase:

Perhaps slelievre can test this on Cygwin - if you specify in some detail what to look for.

We just need to know if this works,

sage: import tempfile
sage: p = plot(sin(x), x, (-pi, pi))
sage: with tempfile.NamedTemporaryFile(suffix=".png") as f:
....:     p.save(filename=f.name)
....: 

or if the fact that save() reopens the (already open) file causes problem.

comment:20 Changed 8 months ago by git

Commit: de4a00ee85328833e7ce679a7c86be9aee3e489f989a4cb43fc711eef66b75844b375ed1a7decf81

Branch pushed to git repo; I updated commit sha1. New commits:

989a4cbTrac #33213: clean up after tmp_filename() and tmp_dir() for now.

comment:21 Changed 8 months ago by Michael Orlitzky

That last commit should avoid most leftover junk in the meantime, too. It's easy so why not.

comment:22 Changed 8 months ago by Dima Pasechnik

@was - this ticket, and its dependency #8784, are also to properly fix #33027

comment:23 Changed 8 months ago by William Stein

Hi everybody - thanks for the comments. I just want to be clear that I am in no way suggesting blocking this ticket moving forward with a positive review. I just want to record my misgivings here, so they may be helpful for anybody that might hit problems as a result. I'm not a fan of this stupid sage-cleaner hack that I wrote long ago, and I'm happy to see it get deleted.

comment:24 Changed 8 months ago by Matthias Köppe

The changes to using TemporaryDirectory in doctests are easy to review and are definitely a great improvement that we should merge as soon as possible.

However, I would suggest to split this ticket, so that the wholesale removal of the cleaner is in its own ticket. In particular, user code may continue to use SAGE_TMP, as this was the advertised interface since the beginning of time. So we need a transition period here. Perhaps a deprecation warning that the cleaner issues when it actually finds something to clean.

comment:25 in reply to:  24 ; Changed 8 months ago by Michael Orlitzky

Replying to mkoeppe:

The changes to using TemporaryDirectory in doctests are easy to review and are definitely a great improvement that we should merge as soon as possible.

However, I would suggest to split this ticket, so that the wholesale removal of the cleaner is in its own ticket. In particular, user code may continue to use SAGE_TMP, as this was the advertised interface since the beginning of time. So we need a transition period here. Perhaps a deprecation warning that the cleaner issues when it actually finds something to clean.

To my surprise, SAGE_TMP was actually undocumented. It's not in the reference manual or the developer guide, and it had no docstring. But you're probably right that a few sage developers managed to use it in their own code.

If we want to raise a deprecation warning for it, I can do so in a way that doesn't affect the rest of this ticket by changing SAGE_TMP to return a tempfile.TemporaryDirectory() instead of the old DOT_SAGE value. Then its use won't create anything that would require sage-cleaner to remove.

comment:26 in reply to:  25 ; Changed 8 months ago by Matthias Köppe

I'm still -1 on removing the sage-cleaner here on the same ticket.

In particular I don't think the explanation why the process-killing part of it should suddenly no longer be necessary is very convincing. Let's please not introduce big changes like this without a proper transition plan.

comment:27 in reply to:  26 Changed 8 months ago by Michael Orlitzky

Replying to mkoeppe:

I'm still -1 on removing the sage-cleaner here on the same ticket.

In particular I don't think the explanation why the process-killing part of it should suddenly no longer be necessary is very convincing.

That's mainly because of #8784.

comment:28 Changed 8 months ago by Matthias Köppe

Let's confirm this in practice please by doing the deprecation warnings whenever the cleaner kills or deletes something.

comment:29 in reply to:  28 ; Changed 8 months ago by Michael Orlitzky

Replying to mkoeppe:

Let's confirm this in practice please by doing the deprecation warnings whenever the cleaner kills or deletes something.

It doesn't really delete anything except the files created specifically for the benefit of sage-cleaner, and some legacy paths whose locations were changed in the fix_old_mistakes() funtion.

I'm not strongly opposed to leaving sage-cleaner alone, but a deprecation warning on killed processes is going to produce false positives on every process that would have terminated nicely but was killed prematurely by sage-cleaner, i.e. most of them.

comment:30 Changed 8 months ago by Dima Pasechnik

Perhaps it would be interesting to know whether after the changes (but keeping the cleaner in) the cleaner would still have anything to kill. (my understanding it starts after Sage terminates, so most likely it won't have to do anything)

comment:31 in reply to:  29 Changed 8 months ago by Matthias Köppe

Replying to mjo:

Replying to mkoeppe:

whenever the cleaner [...] deletes something.

It doesn't really delete anything except the files created specifically for the benefit of sage-cleaner

You are right, thanks.

comment:32 in reply to:  18 ; Changed 8 months ago by Samuel Lelièvre

Replying to dimpase:

Perhaps slelievre can test this on Cygwin

My attempts to build 9.5 beta and rc releases fail on sagemath_doc_html.

My attempts to build the 9.6 beta releases fail earlier, on sagelib I think.

comment:33 in reply to:  32 ; Changed 8 months ago by Michael Orlitzky

Replying to slelievre:

My attempts to build the 9.6 beta releases fail earlier, on sagelib I think.

Fortunately this is only a python question... if you don't mind, does this work?

>>> import tempfile
>>> with tempfile.NamedTemporaryFile(suffix=".txt") as f:
...     with open(f.name, "w") as g:
...         _ = g.write("Hello, world!")
...     with open(f.name) as h:
...         contents = h.read()
... 
>>> print(contents)
Hello, world!

comment:34 in reply to:  24 Changed 8 months ago by Matthias Köppe

Replying to mkoeppe:

The changes to using TemporaryDirectory in doctests are easy to review and are definitely a great improvement that we should merge as soon as possible.

However, I would suggest to split this ticket

I've opened #33383 for the easy to review changes in the doctests.

Last edited 8 months ago by Matthias Köppe (previous) (diff)

comment:35 in reply to:  33 ; Changed 7 months ago by Samuel Lelièvre

Replying to mjo:

does this work?

>>> import tempfile
>>> with tempfile.NamedTemporaryFile(suffix=".txt") as f:
...     with open(f.name, "w") as g:
...         _ = g.write("Hello, world!")
...     with open(f.name) as h:
...         contents = h.read()
... 
>>> print(contents)
Hello, world!

Yes, it works in Cygwin with Python 3.7.12, Python 3.8.12, Python 3.9.10.

comment:36 in reply to:  35 Changed 7 months ago by Michael Orlitzky

Replying to slelievre:

Yes, it works in Cygwin with Python 3.7.12, Python 3.8.12, Python 3.9.10.

Thank you! That means I can go back and simplify a lot of stuff.

comment:37 Changed 7 months ago by git

Commit: 989a4cb43fc711eef66b75844b375ed1a7decf81b9101fd945428f71ef3873df924496be0c211067

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

56ebd57Trac #33213: don't delete SAGE_TMP upon exiting.
c6b6fb8Trac #33213: remove SAGE_TMP from sage.misc.all.
888aeb3Trac #33213: replace SAGE_TMP in cleaner/quit interfaces.
f43965bTrac #33213: drop SAGE_TMP from sage.misc.misc.
b9d1746Trac #33213: remove sage.interfaces.cleaner.
e445eb8Trac #33213: close an open file a bit sooner in gap interface.
049ce98Trac #33213: use @cached_function for the spawned processes file.
e289f0bTrac #33213: ensure temporary gap workspace is created in a doctest.
7dd9ff0Trac #33213: clean up after tmp_filename() and tmp_dir() for now.
b9101fdTrac #33213: use old SAGE_TMP path for spawned_processes file.

comment:38 Changed 7 months ago by Michael Orlitzky

Dependencies: #878432987
Description: modified (diff)
Status: needs_reviewneeds_work

comment:39 Changed 7 months ago by Michael Orlitzky

Dependencies: 32987#32987

comment:40 Changed 7 months ago by git

Commit: b9101fd945428f71ef3873df924496be0c21106786e654bb37ea223e62976a268c2b66107fe30e49

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

518b8ccTrac #33213: don't delete SAGE_TMP upon exiting.
754be97Trac #33213: remove SAGE_TMP from sage.misc.all.
8c99287Trac #33213: replace SAGE_TMP in cleaner/quit interfaces.
e2687dcTrac #33213: drop SAGE_TMP from sage.misc.misc.
2ef1cb8Trac #33213: remove sage.interfaces.cleaner.
98722baTrac #33213: close an open file a bit sooner in gap interface.
69e0f92Trac #33213: use @cached_function for the spawned processes file.
6534cc1Trac #33213: ensure temporary gap workspace is created in a doctest.
e4f0797Trac #33213: clean up after tmp_filename() and tmp_dir() for now.
86e654bTrac #33213: use old SAGE_TMP path for spawned_processes file.

comment:41 Changed 7 months ago by Michael Orlitzky

Status: needs_workneeds_review

Ok, back to patchbots.

This branch is based on #32987 which deprecates sage_makedirs (also now redundant), and does not remove sage-cleaner. We can do that in another ticket.

comment:42 Changed 7 months ago by Matthias Köppe

Authors: Michael Orlitzky

comment:43 Changed 7 months ago by Matthias Köppe

This one needs to be reverted:

diff --git a/src/doc/en/reference/interfaces/index.rst b/src/doc/en/reference/interfaces/index.rst
index d007f67..f09fc62 100644
--- a/src/doc/en/reference/interfaces/index.rst
+++ b/src/doc/en/reference/interfaces/index.rst
@@ -108,7 +108,6 @@ and testing to make sure nothing funny is going on).
    sage/interfaces/tachyon
    sage/interfaces/tides
 
-   sage/interfaces/cleaner
    sage/interfaces/quit
    sage/interfaces/read_data
 

comment:44 Changed 7 months ago by Matthias Köppe

Should tempfile become a global import (sage.all) to make the repeated imports in doctests unnecessary?

comment:45 Changed 7 months ago by Matthias Köppe

Also src/sage/interfaces/expect.py still has cleaner-related changes

comment:46 Changed 7 months ago by git

Commit: 86e654bb37ea223e62976a268c2b66107fe30e49e6126787dec8007c9af93262d0b9c976d71b6b0e

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

72ed289Trac #33213: move sage.interfaces.cleaner.cleaner() to quit interface.
0a75831Trac #33213: close an open file a bit sooner in gap interface.
6b7322bTrac #33213: use @cached_function for the spawned processes file.
c70d5f6Trac #33213: ensure temporary gap workspace is created in a doctest.
ca764c6Trac #33213: clean up after tmp_filename() and tmp_dir() for now.
e612678Trac #33213: use old SAGE_TMP path for spawned_processes file.

comment:47 Changed 7 months ago by git

Commit: e6126787dec8007c9af93262d0b9c976d71b6b0eea321c961ffd41ca483c9b489b59d064cf203b1c

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

a0cdbcdTrac #33213: move sage.interfaces.cleaner.cleaner() to quit interface.
e0961afTrac #33213: close an open file a bit sooner in gap interface.
2123287Trac #33213: use @cached_function for the spawned processes file.
50cf662Trac #33213: ensure temporary gap workspace is created in a doctest.
465281aTrac #33213: clean up after tmp_filename() and tmp_dir() for now.
ea321c9Trac #33213: use old SAGE_TMP path for spawned_processes file.

comment:48 in reply to:  43 Changed 7 months ago by Michael Orlitzky

Replying to mkoeppe:

This one needs to be reverted:

Thanks, done. I also removed the cleaner() function from sage.interfaces.cleaner, since it was replaced by register_spawned_process() in sage.interfaces.quit.

comment:49 in reply to:  44 ; Changed 7 months ago by Michael Orlitzky

Replying to mkoeppe:

Should tempfile become a global import (sage.all) to make the repeated imports in doctests unnecessary?

I don't have a strong opinion either way. The main downside is that it would clutter the global namespace for something that is pretty much only used for doctests. On the other hand, if we import it automatically only during doctesting, then the examples can't be copy & pasted.

comment:50 in reply to:  45 Changed 7 months ago by Michael Orlitzky

Replying to mkoeppe:

Also src/sage/interfaces/expect.py still has cleaner-related changes

Which one? The change from cleaner() to register_spawned_process() is intentional, and will make it easier to remove the cleaner in the future.

comment:51 in reply to:  49 Changed 7 months ago by Matthias Köppe

Replying to mjo:

if we import it automatically only during doctesting

definitely we shouldn't do that

comment:52 Changed 6 months ago by Matthias Köppe

Status: needs_reviewneeds_work

comment:53 Changed 6 months ago by git

Commit: ea321c961ffd41ca483c9b489b59d064cf203b1ce8b8fa568a2cbbca6001f8890dd726c0c2c4882c

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

8c1ea7dTrac #33213: don't delete SAGE_TMP upon exiting.
c83b811Trac #33213: remove SAGE_TMP from sage.misc.all.
c31d7d5Trac #33213: replace SAGE_TMP in cleaner/quit interfaces.
d86a3e6Trac #33213: drop SAGE_TMP from sage.misc.misc.
ac0dea7Trac #33213: move sage.interfaces.cleaner.cleaner() to quit interface.
b268fd9Trac #33213: close an open file a bit sooner in gap interface.
8ce0515Trac #33213: use @cached_function for the spawned processes file.
3bfd7f9Trac #33213: ensure temporary gap workspace is created in a doctest.
9f8a2f1Trac #33213: clean up after tmp_filename() and tmp_dir() for now.
e8b8fa5Trac #33213: use old SAGE_TMP path for spawned_processes file.

comment:54 Changed 6 months ago by Michael Orlitzky

Status: needs_workneeds_review

comment:55 Changed 6 months ago by Matthias Köppe

sage -t --long --random-seed=286814129660227055850621679081389420825 src/sage/misc/latex.py
**********************************************************************
File "src/sage/misc/latex.py", line 1970, in sage.misc.latex.?
Failed example:
    with tempfile.NamedTemporaryFile(suffix=".png") as f:
        png(ZZ[x], f.name) # random, optional - latex imagemagick
Exception raised:

the # optional is on the wrong line

comment:56 Changed 6 months ago by Matthias Köppe

Status: needs_reviewneeds_work

comment:57 Changed 6 months ago by git

Commit: e8b8fa568a2cbbca6001f8890dd726c0c2c4882ca3b58ad4771287bfcc7f2952a56b67b3e075daaa

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

63ac9b8Trac #33213: don't delete SAGE_TMP upon exiting.
e191da6Trac #33213: remove SAGE_TMP from sage.misc.all.
0498a50Trac #33213: replace SAGE_TMP in cleaner/quit interfaces.
b90a7ebTrac #33213: drop SAGE_TMP from sage.misc.misc.
279fe33Trac #33213: move sage.interfaces.cleaner.cleaner() to quit interface.
c9b00b8Trac #33213: close an open file a bit sooner in gap interface.
e4a82e7Trac #33213: use @cached_function for the spawned processes file.
063b3feTrac #33213: ensure temporary gap workspace is created in a doctest.
386b6a5Trac #33213: clean up after tmp_filename() and tmp_dir() for now.
a3b58adTrac #33213: use old SAGE_TMP path for spawned_processes file.

comment:58 Changed 6 months ago by Michael Orlitzky

Status: needs_workneeds_review

Thanks, fixed.

comment:59 Changed 6 months ago by Matthias Köppe

--- a/src/sage/repl/attach.py
+++ b/src/sage/repl/attach.py
@@ -170,12 +170,14 @@ def load_attach_path(path=None, replace=False):
...
+    We put a new, empty direcoty on the attach path for testing

"directory"

comment:60 Changed 6 months ago by Matthias Köppe

Cc: Travis Scrimshaw added

Overall, I'm a bit unsure about changes like this one to the doctests. What is lost for the user is being able to copy-paste the code and then inspect the generated file.

diff --git a/src/sage/numerical/mip.pyx b/src/sage/numerical/mip.pyx
index d966be1..4bf61d1 100644
--- a/src/sage/numerical/mip.pyx
+++ b/src/sage/numerical/mip.pyx
@@ -1336,7 +1336,9 @@ cdef class MixedIntegerLinearProgram(SageObject):
             sage: x = p.new_variable(nonnegative=True)
             sage: p.set_objective(x[1] + x[2])
             sage: p.add_constraint(-3*x[1] + 2*x[2], max=2,name="OneConstraint")
-            sage: p.write_mps(os.path.join(SAGE_TMP, "lp_problem.mps"))
+            sage: import tempfile
+            sage: with tempfile.NamedTemporaryFile(suffix=".mps") as f:
+            ....:     p.write_mps(f.name)
             Writing problem data to ...
             17 records were written

comment:61 Changed 6 months ago by git

Commit: a3b58ad4771287bfcc7f2952a56b67b3e075daaa0c59d5d1e5ed73ec2332a130054e287a4c346919

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

eb1bd48Trac #33213: don't delete SAGE_TMP upon exiting.
87d50beTrac #33213: remove SAGE_TMP from sage.misc.all.
7434ae0Trac #33213: replace SAGE_TMP in cleaner/quit interfaces.
738a684Trac #33213: drop SAGE_TMP from sage.misc.misc.
7bf0a5dTrac #33213: move sage.interfaces.cleaner.cleaner() to quit interface.
74dfbd5Trac #33213: close an open file a bit sooner in gap interface.
f27612fTrac #33213: use @cached_function for the spawned processes file.
d097e2fTrac #33213: ensure temporary gap workspace is created in a doctest.
3b16d2eTrac #33213: clean up after tmp_filename() and tmp_dir() for now.
0c59d5dTrac #33213: use old SAGE_TMP path for spawned_processes file.

comment:62 in reply to:  59 Changed 6 months ago by Michael Orlitzky

Replying to mkoeppe:

"directory"

also fixed

comment:63 in reply to:  60 Changed 6 months ago by Michael Orlitzky

Replying to mkoeppe:

Overall, I'm a bit unsure about changes like this one to the doctests. What is lost for the user is being able to copy-paste the code and then inspect the generated file.

In many cases, the point of the method is that it writes what would normally be shown to the user to a file instead. If you want to see this MILP, for example, then p.show() can be used instead. I'm dodging your point, but looking back, many of these tests are in the same spirit.

In general, I would say it's bad practice to give users code that will write to a temporary file and teach them to expect it to remain there. It's better to have these copy/paste examples fail immediately, so that the user will pick a non-temporary path for his own experiments. Otherwise he could lose some work when sage exits or when the machine reboots.

comment:64 Changed 6 months ago by Matthias Köppe

Milestone: sage-9.6sage-9.7

comment:65 Changed 5 months ago by git

Commit: 0c59d5d1e5ed73ec2332a130054e287a4c34691920bc0cfeb5a007022d010e5fb300ffcb304ee770

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

08430e0Trac #33213: don't delete SAGE_TMP upon exiting.
058907eTrac #33213: remove SAGE_TMP from sage.misc.all.
ad1dbd8Trac #33213: replace SAGE_TMP in cleaner/quit interfaces.
f57fc07Trac #33213: drop SAGE_TMP from sage.misc.misc.
aacc73cTrac #33213: move sage.interfaces.cleaner.cleaner() to quit interface.
615f6e5Trac #33213: close an open file a bit sooner in gap interface.
9f7d581Trac #33213: use @cached_function for the spawned processes file.
9843b64Trac #33213: ensure temporary gap workspace is created in a doctest.
47f9294Trac #33213: clean up after tmp_filename() and tmp_dir() for now.
20bc0cfTrac #33213: use old SAGE_TMP path for spawned_processes file.

comment:66 Changed 5 months ago by François Bissey

Cc: François Bissey added

comment:67 Changed 5 months ago by Matthias Köppe

Dependencies: #32987#33779

I've split out #33779: Remove use of SAGE_TMP in sage.interfaces.latte

comment:68 in reply to:  25 Changed 5 months ago by Matthias Köppe

Replying to mjo:

Replying to mkoeppe:

user code may continue to use SAGE_TMP, as this was the advertised interface since the beginning of time. So we need a transition period here. [...]

To my surprise, SAGE_TMP was actually undocumented. It's not in the reference manual or the developer guide, and it had no docstring. But you're probably right that a few sage developers managed to use it in their own code.

Current branch still removes SAGE_TMP. This needs to be backed out. Likewise the removal of ECL_TMP, SPYX_TMP - they are part of the public API.

comment:69 Changed 5 months ago by git

Commit: 20bc0cfeb5a007022d010e5fb300ffcb304ee7705a9edd957b014a3445aefd59313d3435bc5d4dda

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

a3ecdceTrac #33213: don't delete SAGE_TMP upon exiting.
f6cb517Trac #33213: remove SAGE_TMP from sage.misc.all.
7e0e3c0Trac #33213: replace SAGE_TMP in cleaner/quit interfaces.
218055bTrac #33213: deprecate SAGE_TMP.
66268d6Trac #33213: move sage.interfaces.cleaner.cleaner() to quit interface.
a373c84Trac #33213: close an open file a bit sooner in gap interface.
ccbe766Trac #33213: use @cached_function for the spawned processes file.
81a3bd8Trac #33213: ensure temporary gap workspace is created in a doctest.
06202e3Trac #33213: clean up after tmp_filename() and tmp_dir() for now.
5a9edd9Trac #33213: use old SAGE_TMP path for spawned_processes file.

comment:70 Changed 5 months ago by Michael Orlitzky

I replaced the removal of SAGE_TMP by a deprecation, and likewise with the rename SPYX_TMP -> spyx_tmp().

I don't fully agree with doing the same for ECL_TMP since sage doesn't use it any more: anyone using it will have their code implicitly broken rather than explicitly. Nevertheless, I doubt anyone is using it, so I've put it back with a deprecation and relocated the path somewhere under the system's temporary location.

comment:71 Changed 5 months ago by Matthias Köppe

Could you move the new functions spyx_tmp to sage.misc.temporary_file, like I tried in #32986? This would help with modularization

comment:72 Changed 5 months ago by Matthias Köppe

I've opened #33790 for the ECL/Maxima stuff - I'd like to run this by the ECL/Maxima people

comment:73 Changed 5 months ago by git

Commit: 5a9edd957b014a3445aefd59313d3435bc5d4dda1a4968523eb9c2ecd66e7148e78e66db909c86ae

Branch pushed to git repo; I updated commit sha1. New commits:

1a49685Trac #33213: move spyx_tmp() to sage.misc.temporary_file.

comment:74 Changed 5 months ago by Matthias Köppe

Thanks. I've opened #33793 for this

comment:75 Changed 5 months ago by Matthias Köppe

I forgot to say I'll probably want to keep sage.misc.temporary_file without a dependence on Cython modules such as cachefunc

comment:76 Changed 5 months ago by Michael Orlitzky

Dependencies: #33779#33779 #33793

comment:77 Changed 5 months ago by git

Commit: 1a4968523eb9c2ecd66e7148e78e66db909c86aea980ff2bb7b3ab895f25f60e11024ea225899cfc

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

8c3da84Trac #33213: don't delete SAGE_TMP upon exiting.
8fa1aa6Trac #33213: remove SAGE_TMP from sage.misc.all.
e12408aTrac #33213: replace SAGE_TMP in cleaner/quit interfaces.
a899a3fTrac #33213: deprecate SAGE_TMP.
c810bf0Trac #33213: move sage.interfaces.cleaner.cleaner() to quit interface.
f689fe3Trac #33213: close an open file a bit sooner in gap interface.
0a39999Trac #33213: use @cached_function for the spawned processes file.
54a49c4Trac #33213: ensure temporary gap workspace is created in a doctest.
93d9b4cTrac #33213: clean up after tmp_filename() and tmp_dir() for now.
a980ff2Trac #33213: use old SAGE_TMP path for spawned_processes file.

comment:78 Changed 5 months ago by Matthias Köppe

Opened #33797, #33799

comment:79 Changed 5 months ago by Michael Orlitzky

Dependencies: #33779 #33793#33779 #33793 #33797 #33799

comment:80 Changed 5 months ago by Matthias Köppe

Opened #33829, #33837

Last edited 5 months ago by Matthias Köppe (previous) (diff)

comment:81 Changed 4 months ago by Dima Pasechnik

Dependencies: #33779 #33793 #33797 #33799#33829

comment:82 Changed 4 months ago by Dima Pasechnik

Branch: u/mjo/ticket/33213u/dimpase/ticket/33213
Commit: a980ff2bb7b3ab895f25f60e11024ea225899cfc722f56c6c9c961c9c4dcb3e956c61c2b9b16def5
Status: needs_reviewneeds_work

need to get rid of SAGE_TMP in msolve.py

src/sage/rings/polynomial/msolve.py:from sage.misc.all import SAGE_TMP
src/sage/rings/polynomial/msolve.py:    msolve_in = tempfile.NamedTemporaryFile(dir=SAGE_TMP, mode='w',
...

Last 10 new commits:

9ee2c0dTrac #33213: remove mentions of SAGE_TMP from the lazy_string module.
bab61dfTrac #33213: don't delete SAGE_TMP upon exiting.
9d56a0bTrac #33213: remove SAGE_TMP from sage.misc.all.
95abd2bTrac #33213: replace SAGE_TMP in cleaner/quit interfaces.
e875f1dTrac #33213: deprecate SAGE_TMP.
7dd2ee0Trac #33213: move sage.interfaces.cleaner.cleaner() to quit interface.
e386ceeTrac #33213: close an open file a bit sooner in gap interface.
595c082Trac #33213: use @cached_function for the spawned processes file.
46ae7b2Trac #33213: ensure temporary gap workspace is created in a doctest.
722f56cTrac #33213: use old SAGE_TMP path for spawned_processes file.

comment:83 Changed 4 months ago by Dima Pasechnik

rebased over the latest 9.7.beta1

comment:84 Changed 4 months ago by git

Commit: 722f56c6c9c961c9c4dcb3e956c61c2b9b16def5beacc6853a162550cfd5cfd3ba9b004a181d5a6c

Branch pushed to git repo; I updated commit sha1. New commits:

beacc68do not use SAGE_TMP

comment:85 Changed 4 months ago by git

Commit: beacc6853a162550cfd5cfd3ba9b004a181d5a6c9afa20b82c54b44740469f37d0b8668280933056

Branch pushed to git repo; I updated commit sha1. New commits:

9afa20badd forgotten while rebasing deprecation in misc/dist.py

comment:86 Changed 4 months ago by Dima Pasechnik

Reviewers: Matthias Koeppe, Dima Pasechnik
Status: needs_workpositive_review

Matthias, is it OK with you?

comment:87 Changed 4 months ago by Matthias Köppe

Is it still on top of #33829?

comment:88 in reply to:  87 Changed 4 months ago by Dima Pasechnik

Replying to mkoeppe:

Is it still on top of #33829?

it is. Is it wrong?

Last edited 4 months ago by Dima Pasechnik (previous) (diff)

comment:89 Changed 4 months ago by Matthias Köppe

Status: positive_reviewneeds_review

I haven't reviewed the changes on the ticket. The parts that I was able to review have been split out to separate tickets.

comment:90 Changed 4 months ago by Matthias Köppe

Dependencies: #33829#33829, #33790
Status: needs_reviewneeds_work
Work issues: Rebase on top of the dependencies

comment:91 Changed 4 months ago by git

Commit: 9afa20b82c54b44740469f37d0b8668280933056d94a1e62c1ce92ab65cd840b3f002101b00a36e5

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

c8c9d40Trac #33213: deprecate SAGE_TMP.
ec1a3cfTrac #33213: move sage.interfaces.cleaner.cleaner() to quit interface.
78db2fdTrac #33213: close an open file a bit sooner in gap interface.
56e2964Trac #33213: use @cached_function for the spawned processes file.
31986c1Trac #33213: ensure temporary gap workspace is created in a doctest.
c7ff254Trac #33213: use old SAGE_TMP path for spawned_processes file.
78f029bdo not use SAGE_TMP
7ae0e41add forgotten while rebasing deprecation in misc/dist.py
5d314d9pkgs/sage-setup, sagemath-{objects,categories,standard}: Add tox environment sagepython
d94a1e6src/doc/en/developer/packaging_sage_library.rst: Use sagepython instead of hardcoded py39

comment:92 Changed 4 months ago by Dima Pasechnik

Status: needs_workneeds_review

comment:93 Changed 4 months ago by Dima Pasechnik

Work issues: Rebase on top of the dependencies

rebased over #33829, #33790 (and 9.7.beta2), all good

comment:94 Changed 4 months ago by Dima Pasechnik

Status: needs_reviewpositive_review

comment:95 Changed 3 months ago by Volker Braun

Branch: u/dimpase/ticket/33213d94a1e62c1ce92ab65cd840b3f002101b00a36e5
Resolution: fixed
Status: positive_reviewclosed

comment:96 Changed 3 months ago by Matthias Köppe

Commit: d94a1e62c1ce92ab65cd840b3f002101b00a36e5

The branch includes a rebased version of #32716

comment:97 Changed 6 days ago by Matthias Köppe

Note: See TracTickets for help on using tickets.