src/sage/misc/latex.py: fix view()

[orlitzky]

Rewriting the view() function to use python's tempfile module instead of sage's own tmp_dir() accidentally broke this function, because the viewer program needs the file to be slightly less temporary than we made it. The viewer would launch, but then view() would immediately return, causing the file that the viewer was about to look for to be deleted.

To fix it, we now launch the viewer program with a coroutine and wait for it (asynchronously) to return. Only when it has returned will we delete the file it's viewing.

Closes: #36526

[EmmanuelCharpentier]

Nope :

view((1/(1+e^-(a*x+b))).diff(x, 2).factor()) does indeed display the requested expression in a PDF viewer windows, but does not return until the viewer window is closed. Large behaviour change, and inconvenient for many purposes.

==> s.needs work

[orlitzky]

Well that's cute. I was sort of expecting the asynchronous subprocess example from the asynchronous I/O documentation to run asynchronously. I just clobbered my sage install by switching to this clean branch but I'll try again as soon as I rebuild sagelib.

[orlitzky]

Attempt number 3.

[EmmanuelCharpentier]

Attempt number 3.

This one does display the expression and returns. Seems a bit slower than the original version, but my system is currently loaded (running ptestlong).

I wait for the end of tests to give positive review...

[EmmanuelCharpentier]

Nice little gag : a Sage console won't quit before the last PDF viewer window is closed.

Similarly, from the (bash) command line, sage -c "view((1/(1+e^-x)).diff(x, 2).factor())" wont't return before the closing of the evince window.

May be a hindrance...

[orlitzky]

I would not have guessed that this was going to be the hardest thing I did today. Attempt number four!

[EmmanuelCharpentier]

I would not have guessed that this was going to be the hardest thing I did today. Attempt number four!

I'll have a look now, but full testing will have to wait tomorrow.

[EmmanuelCharpentier]

Attempt number four!

Well...

• view((1/(1+e^-x)).diff(x, 2).factor()) returns a tad before the viewer window appears ;
• one can quit while the viewer window is open, and it stays open "indefinitely ;
  but
• from the bash prompt, sage -c "view((1/(1+e^-x)).diff(x, 2).factor())" returns but the viewer window does not open...

Sorry !

"Cent fois sur le métier remettez votre ouvrage"...

[EmmanuelCharpentier]

FWIW : attempt number three did not introduce any new ptestlong failure.

HTH,

[github-actions]

Documentation preview for this PR (built with commit a859ac2; changes) is ready! 🎉

[mkoeppe]

Because of https://github.com/sagemath/sage/wiki/Sage-10.2-Release-Tour#open-blocker-prs-are-applied-automatically-in-ci-workflows, let's set this to "blocker" only after the fix works

[orlitzky]

• from the bash prompt, sage -c "view((1/(1+e^-x)).diff(x, 2).factor())" returns but the viewer window does not open...

I just reverted everything, and this never worked to begin with. We have conflicting requirements if we want sage to,

1. be able to exit while the viewer is running, and
2. clean up its temporary files when it exits

When both of those are satisfied, sage -c ... will launch the viewer process but then immediately exit. By the time the viewer has started (which may be two or three processes later), sage has already exited and the temporary file has been removed.

There's a similar problem with plot/show. In that case the solution I've always used is to save() the plot first instead of using show() directly. Maybe in this case everyone would be happy if there was a way to pass a filename to view() so that it doesn't use a temporary file?

[EmmanuelCharpentier]

We have conflicting requirements if we want sage to [...]

Maybe #36436 wasn't such a great idea, after all : the invoked "security reasons" are a tad nebulous to me...

[EmmanuelCharpentier]

Shouldn't view spawn an independent viewer instance ?

BTW : plot has indeed the same problem. I suppose that this was already its default behaviour which, curiously, never bothered me.

[orlitzky]

We have conflicting requirements if we want sage to [...]

Maybe #36436 wasn't such a great idea, after all : the invoked "security reasons" are a tad nebulous to me...

What security reasons? The goal of #36436 is to minimize how much junk sage leaves on the filesystem. For example if you call view() a hundred times and close all of the viewers, you should not have 100 files left in /tmp. We especially want sage to clean up those files when it exits, which is what makes the sage -c ... example impossible.

Shouldn't view spawn an independent viewer instance ?

Do you mean anything specific by independent? In general its independence is limited by using a temporary file created by sage and that needs to be cleaned up by sage.

The current version should not make anything worse, and has the added benefit of cleaning up the files when the viewer is closed.

[EmmanuelCharpentier]

What security reasons?

From your initial message of #36436 :

Since we don't want the output path to be predictable (for security reasons)

Those still are a tad nebulous to my (admittedly slow and retarded) mind.

that needs to be cleaned up

Agreed

by sage

Not necessarily : Possible algorithm :
- create the temporary file
- spawn a viewer opening this file
- (possibly after a "reasonable" delay (how to define this delay is a nice problem...)) ulink the temporary file. The latter being opened by the viewer, it will be deleted only when the said viewer exits.

I'll check the curent behaviour of 10.2.beta7 (IIRC), but I cannot do this before Friday 27.

HTH,

[orlitzky]

Since we don't want the output path to be predictable (for security reasons)

Those still are a tad nebulous to my (admittedly slow and retarded) mind.

#36436 didn't change anything in that regard. I was trying to document why we need a temporary directory instead of a temporary file, but a directory was always used -- just a less temporary one. At first glance, the latex build process looks like it only needs a temporary file because it turns foo.tex into foo.pdf which can then be viewed. But if /tmp/foo.tex exists, there are several bad things another person on the same machine could do to /tmp/foo.pdf before you have a chance to create it.

Just for an example: someone else could create /tmp/foo.pdf so that he owns it. Now when you compile foo.tex, it gets written to a file that you don't own. Before your PDF viewer is launched, the other person (who still owns the file) could replace the contents of the PDF with something bad, like malicious postscript. Or instead the person could make /tmp/foo.pdf a symlink to one of your private files, and then calling view() would overwrite it. Things like that.

tl;dr I didn't want someone to see the code and think "this only needs a temporary file!" and change it.

that needs to be cleaned up

Agreed

by sage

Not necessarily : Possible algorithm : - create the temporary file - spawn a viewer opening this file - (possibly after a "reasonable" delay (how to define this delay is a nice problem...)) ulink the temporary file. The latter being opened by the viewer, it will be deleted only when the said viewer exits.

Should sage refuse to exit for that reasonable delay? :)

[orlitzky]

FWIW the behavior before #36436 is to stick the file in a new temporary directory, launch the viewer, and then forget about it. The temporary directory is only removed when sage exits and either the atexit() hook cleans it up, or sage-cleaner removes it. sage -c ... doesn't work because the directory is removed too quickly.

Relative to that, the current PR makes only an improvement: everything else is the same, but the directory will also be removed when the viewer closes, which is usually sooner.

[EmmanuelCharpentier]

Should sage refuse to exit for that reasonable delay? :)

I was thinking milliseconds (time to be sure tohave a viewer opening (and thus locking) the temporary file

[orlitzky]

Should sage refuse to exit for that reasonable delay? :)

I was thinking milliseconds (time to be sure tohave a viewer opening (and thus locking) the temporary file

I played with this idea, not intending to keep it, but just to see if it would work. On my machine the "viewer" is xdg-open, which is a shell script, so sage has to launch /bin/sh, find xdg-open, run xdg-open, launch exo-open (xdg-open eventually does this), and then finally launch the real PDF viewer which then has to open the temporary file.

That all has to work on old machines under heavy load, so for sleep(N) to work, N has to be annoyingly high. Too high to sit there and wait whenever someone runs view() for sure. You can run e.g. sleep(N); rm file in the background but then that brings us right back to where we started...

[orlitzky]

Can we merge this for 10.2 to get view() working again, and then if you come up with a better solution, I'll promise to review it? I don't want to have broken it completely in the release.

[EmmanuelCharpentier]

Sorry for the delay to answer : I have been ... interrupded, say.

Can we merge this for 10.2 to get view() working again, and then if you come up with a better solution, I'll promise to review it? I don't want to have broken it completely in the release.

I have been able to check against 10.2.beta5 :

• From a Sage console, both view and plot work.
• From command line, neither sage -c "view(...) nor sage -c "plot(...) display anything. That would be new functionality...

Your fourth attempt is therefore a reasonable fix to the initial problem, that can be incorporated in 10.2.

I wonder if the new functionality I envisioned would be useful. It does not semm that it was previously lacking. I might have succumbed to the temptation of wanting anything possible from a console also possible from command line.

Let's say that if I feel this useful, I'll introduce a new issnue. Okay with you ?

Again, thank you very much for your attention.

[orlitzky]

I wonder if the new functionality I envisioned would be useful. It does not semm that it was previously lacking. I might have succumbed to the temptation of wanting anything possible from a console also possible from command line.

Let's say that if I feel this useful, I'll introduce a new issnue. Okay with you ?

I think it would be useful, it's just not clear to me how to accomplish it. I would be happy to have a better solution though. I will probably encounter this same problem a few more times while cleaning up the remaining uses of tmp_dir().

[mantepse]

Works for me (ubuntu 22.04, emacs 27.1)

Martin

[egourgoulhon]

LGTM: when merged into Sage 10.2.rc0, the view() functionality is restored. Thank you for the fix!

[seblabbe]

Sorry to arrive a bit late here, but while working on sage/misc/latex_standalone.py last year, I need to work around a similar issue. I ended it replacing the run command (inherited from the code taken years ago from view) by check_call instead.

See https://github.com/sagemath/sage/blob/develop/src/sage/misc/latex_standalone.py#L745

[orlitzky]

Sorry to arrive a bit late here, but while working on sage/misc/latex_standalone.py last year, I need to work around a similar issue. I ended it replacing the run command (inherited from the code taken years ago from view) by check_call instead.

See https://github.com/sagemath/sage/blob/develop/src/sage/misc/latex_standalone.py#L745

I'm sure there are similar calls all over sage. Hopefully no one reports any problems with this approach and it can be reused or factored out into a separate function.

The calls in latex_standalone.py are still using tmp_filename() which makes the developer's life a lot easier because it doesn't delete the files when the user is done with them. The crux of the issue here was that I wanted to replace tmp_directory() to avoid that -- to clean up the files as soon as possible. I thought this would be a quick refactoring but apparently it's quite tricky to "forget" the process, in the sense that control is returned to the CLI immediately, while still remembering it enough to clean up after it terminates.

[seblabbe]

Ok I see. So I guess what you have done will need to be done also in latex_standalone.py at some point.

[orlitzky]

Ok I see. So I guess what you have done will need to be done also in latex_standalone.py at some point.

Yeah, assuming it works :)

[AndrewMathas]

As I posted in sage-support, I think that there is still an issue here. In more detail, on a mac the following usually leads to the error The document “sage.pdf” could not be opened. The file doesn’t exist.:

from sage.misc.viewer import viewer
viewer.pdf_viewer('open')
view(crystals.LSPaths( RootSystem(['A',4]).weight_space().basis()[1] ) )

Adding a time.sleep(1) to the run_viewer command in sage.misc.latex fixes the problem, but this is the kind of thing that this PR was trying to avoid. I thought that a solution might be to use:

    def run_viewer():
        run([viewer, output_file], capture_output=True)
        #tmp.cleanup()  # put after t.join() below

    # ...but we execute it asynchronously so that view() completes
    # immediately. The "daemon" flag is important because, without it,
    # sage won't quit until the viewer does.
    from threading import Thread
    t = Thread(target=run_viewer, daemon=True)
    t.start()
    t.join() # wait for the thread to exit
    tmp.cleanup()

but, unfortunately, this does not solve my problem. The problem is that the open command exits as soon as it executes the command that actually opens the file, so knowing that open has exited does not tell you that the file has been opened. This said, rather than using open a workaround is to set the PDF viewer equal to the app that open uses, which in my case is:

viewer.pdf_viewer('/Applications/Skim.app/Contents/MacOS/Skim')

Now everything works as expected. So, perhaps it is enough for the documentation to warn against using generic file opening commands, such as open in macosx.

[orlitzky]

The problem is that the open command exits as soon as it executes the command that actually opens the file, so knowing that open has exited does not tell you that the file has been opened.

That's one of the problems I was trying to fix, but I guess it doesn't work the same on macOS or with how open is implemented. On my (linux) machine, "firefox" is a shell script that behaves the same way: it returns immediately while the actual viewer (the real firefox executable) is still loading. The thread is supposed to execute asynchronously, but the run(..., capture_output=True) call within the thread was supposed to wait until the whole thing is done.

Now everything works as expected. So, perhaps it is enough for the documentation to warn against using generic file opening commands, such as open in macosx.

TBH I'd rather actually fix it, even if that means adding a special case for macOS. But it took a lot of experimenting to find a solution that worked here and I don't have access to a mac to try things.

As a last resort, we could always (on macOS) go back to the way things worked before I broke everything. I.e. leave the file there indefinitely until/unless sage closes gracefully. The downside to that is that it potentially leaves junk on the filesystem if sage crashes or if you write a long-running program, but it may be the lesser of two evils.

[AndrewMathas]

I am happy to help troubleshoot on a mac. Unfortunately, I can't give you log in access to one.