Graphics3d.show abuses graphics_filename

[gagern]

Code in ​sage/plot/plot3d/base.pyx in the method Graphics3d.show() makes invalid use of graphics_filename: it generates a name (like sage0.png), then strips the extension and appends various other extensions during the course of the method. But graphics_filename makes no guarantees that these other file names will be unused. In fact chances are pretty good that these files already exist from a different run of that method inside the same cell. So we get clashes there. Perhaps if things change in other places we might even get a security issue due to a race condition here.

I can think of two possible approaches: either call graphics_filename in several places, once for every file name we need to generate. Or create a temporary directory and write all files inside that with fixed names. The former will likely be easier to handle for notebook, but may make the code harder to read. Particularly those cases where some tool will use one file name to derive another file name might be a real pain, like when the applet size is encoded in the file name, or the magic aroud the archive_name for jmol. Will need some good interactive testing to make sure that everything still works after these changes.

[gagern]

Note that only the last two commits are new for this ticket; the rest comes from #16645 and its dependencies.

I tested things both in notebook and interactively. java3d failed in both cases, for which I just filed #16647. Apart from that, everything looks fine. In particular it seems as if I got all the file name interdependencies for jmol right.

---

New commits:

​58b9c46	minimal fix to restore functionality
​16cbd2d	Merge branch 'develop' into ticket/16533-stopgap
​b3e656b	graphics_filename: return a tmp_filename() if not in EMBEDDED_MODE
​69b285d	Restore possibly positional arguments.
​e3c5542	Merge branch ticket/15515 into ticket/16608 to create ticket/16645.
​9f30e94	Always use graphics_filename instead of tmp_filename.
​bb7307f	Prevent doctest from leaking file into current working directory.
​0dffab6	Include dot in extension for graphics_filename().
​9dfe133	Generate names for 3d graphics files independently.

[gagern]

​These are the changes that need reviewing in this ticket here.

[git]

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

​301185b	Include dot in extension for graphics_filename().
​cf63c2d	Generate names for 3d graphics files independently.

[git]

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

​61809d6	Include dot in extension for graphics_filename().
​84a2f30	Generate names for 3d graphics files independently.

[git]

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

​1a48c6b	Use "with open() as f" syntax
​49ce20b	Set fg to figsize[0] like before

[jdemeyer]

Merged with 6.4.beta4 and added some fixes. Please review.

[git]

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

​463eb63

Rendering canvas3d in all doctests is too slow

[git]

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

​711127f

Merge remote-tracking branch 'origin/develop' into ticket/16640

[kcrisman]

Hi Jeroen: Your first two commits are fine, as is the first commit of Martin's. (Currently looking at his main, second one.) How much slower did the doctests become without your last commit? (Martin, if you see this, what was your thinking on changing the doctest mode?)

[gagern]

Hi, sorry I didn't respond to the commits by Jeroen any sooner.

Replying to kcrisman:

Hi Jeroen: Your first two commits are fine,

I agree. Thanks a lot!

(Martin, if you see this, what was your thinking on changing the doctest mode?)

My main reason for modifying that line at all was the fact that canvas3d only works in embedded mode, so having it available otherwise might confuse users. My reson for including it in doctests is simply because all the other possible viewers are active in doctest mode as well. There is a doctest specifically for the canvas3d case, but if I restrict canvas3d to embedded mode only, that one would fail. So staying in line with all the other tests seemed the reasonable thing to do.

If things really are too slow, I'd change that to if viewer == 'canvas3d' and (DOCTEST_MODE or EMBEDDED_MODE): so we can have that one explicit doctest nevertheless. But first I'd like to quantify that effect on test execution time, and right now I have a build problem.

[jdemeyer]

Always doing canvas3d is really too slow, I remember one file(!) taking over 1000s to doctest.

[git]

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

​bd1479d

Make viewer="canvas3d" an error outside the Notebook

[jdemeyer]

Replying to gagern:

My main reason for modifying that line at all was the fact that canvas3d only works in embedded mode, so having it available otherwise might confuse users.

Fixed by raising a suitable exception.

[kcrisman]

My main reason for modifying that line at all was the fact that canvas3d only works in embedded mode, so having it available otherwise might confuse users.

Fixed by raising a suitable exception.

That was something we needed a while ago anyway! Thanks.

[kcrisman]

Here is something minor but still needing attention. The documentation says

        -  ``filename`` -- string (default: a temp file); file
           to save the image to

but of course this is now only going to be true in the command line. Which probably makes sense. But this should be noted. Similarly,

        # Tachyon resolution options

should probably just add a quick note that "mode defaults" also are being set in the next few lines.

Interestingly,

            # Temporary hack: encode the desired applet size in the end of the filename:
            # (This will be removed once we have dynamic resizing of applets in the browser.)

and the following code could in principle be removed, as the update does indeed have dynamic resizing! But I think let's leave that be for somewhere else; I don't want to mess with the already messy naming of jmol stuff...

Finally, I don't understand this comment - I'm sorry if I am just thinking inside the box:

                # when testing this, modify graphics_filename so that it
                # returns files which don't automatically share a base name,
                # e.g.: "import random; i = random.randint(0, 10000)"

Was this just a "note to self" or do you mean that, as a tester, I should actually change graphics_filename instead of letting it just keep adding one to base names, or ... ? I figured the whole point of this change was to test where they did share a base name, but again I may be thinking too conventionally.

[kcrisman]

So far it seems fine, though I rediscovered and diagnosed #11275 along the way.

[kcrisman]

The following in the command line don't seem to create any files (I mean visible ones in my directory). Maybe they aren't supposed to?

sage: var('y')
y
sage: plot3d(x^2+y^3,(x,-1,1),(y,-1,1),filename='foo')
sage: plot3d(x^2+y^3,(x,-1,1),(y,-1,1),filename='foo',viewer='tachyon')

Note that the second thing pops up a jmol viewer. I can't figure out if this ticket introduced it, though it seems to predate that, based on my experimentation.

Also, with the current documentation, there is no example of how to properly use the filename keyword here, and that should be added.

Anyway, "needs work" to clarify/fix these filename use issues.

[jdemeyer]

Replying to kcrisman:

sage: var('y')
y
sage: plot3d(x^2+y^3,(x,-1,1),(y,-1,1),filename='foo')
sage: plot3d(x^2+y^3,(x,-1,1),(y,-1,1),filename='foo',viewer='tachyon')

The correct syntax is

sage: plot3d(x^2+y^3,(x,-1,1),(y,-1,1)).show(filename='foo')

[jdemeyer]

Replying to kcrisman:

Here is something minor but still needing attention. The documentation says

        -  ``filename`` -- string (default: a temp file); file
           to save the image to

but of course this is now only going to be true in the command line. Which probably makes sense.

No, it doesn't make sense. An explicit filename should override the default, even in the notebook.

[kcrisman]

sage: var('y')
y
sage: plot3d(x^2+y^3,(x,-1,1),(y,-1,1),filename='foo')
sage: plot3d(x^2+y^3,(x,-1,1),(y,-1,1),filename='foo',viewer='tachyon')

The correct syntax is

sage: plot3d(x^2+y^3,(x,-1,1),(y,-1,1)).show(filename='foo')

Good to know. But that sort of proves my point, then, that the documentation (old and new) doesn't tell me that - in this case, because so many keywords can be passed to the plot and then show up in show(). Such as what worked in Sage 6.2 and possibly broken by the new display hook

sage: var('y')
y
sage: plot3d(x^2+y^3,(x,-1,1),(y,-1,1),viewer='jmol')  # jmol applet pops up
sage: plot3d(x^2+y^3,(x,-1,1),(y,-1,1),viewer='tachyon') # tachyon-generated png pops up in a viewer

If you confirm that this does work properly to generate the filenames in the command line then I'm okay with making sure there are explicit examples that say it doesn't work from the plot command - preferably in the plot command docs!

[kcrisman]

Here is something minor but still needing attention. The documentation says

        -  ``filename`` -- string (default: a temp file); file
           to save the image to

but of course this is now only going to be true in the command line. Which probably makes sense.

No, it doesn't make sense. An explicit filename should override the default, even in the notebook.

Hmm, I guess. But I don't know that there is any easy way to find files in a cell directory in the notebook - though I guess a good use case is to save it in the DATA directory, where it of course shouldn't show up in the cell output.

[jdemeyer]

Any files created in the current directory are picked up by the notebook, so it would actually work!

[kcrisman]

Any files created in the current directory are picked up by the notebook, so it would actually work!

Sorry, I wasn't clear. I meant that the current code in this ticket doesn't allow the filenames, and if one enabled it, it would be kind of useless since there is no access to the cell directory. My counterexample to that is that one might want to use filename=DATA+'foo' or something to put a graphic file in the data directory, though I haven't tested whether this actually works.

[jdemeyer]

(never mind)

[gagern]

Replying to kcrisman:

Finally, I don't understand this comment - I'm sorry if I am just thinking inside the box:

                # when testing this, modify graphics_filename so that it
                # returns files which don't automatically share a base name,
                # e.g.: "import random; i = random.randint(0, 10000)"

Was this just a "note to self" or do you mean that, as a tester, I should actually change graphics_filename instead of letting it just keep adding one to base names, or ... ?

Doctests mostly check whether a given file can be created, but to check whether that file displays as intended, you'd use an interactive setup not a doctest. So I envision testers using the notebook to run tests, with no way to programmatically tell whether he is doing tests or something else.

And the notebook numbers files by incrementing some counter until they are unique. I.e. starting by sage0.png then sage1.png. But the critical point here is that the collision detection takes file extensions into account. So if I were to request e.g. a .foo file and a .bar file, I'd get sage0.foo and sage0.bar not sage1.bar. Usually. Unless sage0.bar already exists at the time the graphics file name is requested, in which case I'd get sage0.foo but sage1.bar.

The problematic case I had in mind was one where some viewer uses one file name to derive some other file name. So if a viewer were to open sage0.foo and from that name infer that it also has to open sage0.bar, then this would work during test cases, but might fail unexpectedly if a user creates sage0.bar in some other code inside the same cell.

I'd like to detect such situations. I'd like to make sure that if there is more than one file required for some viewer, then all file names are stated explicitly somewhere, and not inferred by some extension replacement. And that's the point of the comment. I'd like to break the sequential numbering and replace it by random numbers to ensure that inferred file names would break.

So yes, anyone who tests a patch that might be affected by the scenario outlined above should, in my opinion, temporarily modify the code of graphics_filename to ensure that no file name inference is used. If you can think of a better wording to convey this idea, please let me know. I guess we could also add a link to this comment here, since the whole explanation is too long to put into such a comment, imho.

[jdemeyer]

In vanilla (i.e. without this patch) sage-6.4.rc0 in the notebook, when doing

cube(viewer="jmol")

I get

/usr/local/src/sage-config/local/lib/python2.7/site-packages/IPython/cor\
e/formatters.py:239: FormatterWarning: Exception in text/plain
formatter: Jmol failed to create file
'/tmp/tmpE8eQHg/.jmol_images/sage0-size500.jmol.png', see
'/home/jdemeyer/.sage/temp/tamiyo/24593/tmp_rBkFiX.txt' for details
  FormatterWarning,
None

(not the clearest error message) followed by a blank red square.

Looking at the file mentioned for details gives

Exception in thread "main" java.lang.UnsupportedClassVersionError: org/jmol/translation/Jmol/en_GB/Messages_en_GB : Unsupported major.minor version 51.0
        at java.lang.ClassLoader.defineClass1(Native Method)
        at java.lang.ClassLoader.defineClass(ClassLoader.java:634)
        at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142)
        at java.net.URLClassLoader.defineClass(URLClassLoader.java:277)
        at java.net.URLClassLoader.access$000(URLClassLoader.java:73)
        at java.net.URLClassLoader$1.run(URLClassLoader.java:212)
        at java.security.AccessController.doPrivileged(Native Method)
        at java.net.URLClassLoader.findClass(URLClassLoader.java:205)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:321)
        at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:294)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:266)
        at java.lang.Class.forName0(Native Method)
        at java.lang.Class.forName(Class.java:188)
        at org.jmol.api.Interface.getInterface(Unknown Source)
        at org.jmol.i18n.Resource.getResource(Unknown Source)
        at org.jmol.i18n.GT.addBundle(Unknown Source)
        at org.jmol.i18n.GT.addBundles(Unknown Source)
        at org.jmol.i18n.GT.<init>(Unknown Source)
        at org.jmol.i18n.GT.getTextWrapper(Unknown Source)
        at org.jmol.i18n.GT._(Unknown Source)
        at org.openscience.jmol.app.JmolApp.getOptions(Unknown Source)
        at org.openscience.jmol.app.JmolApp.parseCommandLine(Unknown Source)
        at org.openscience.jmol.app.JmolData.main(Unknown Source)

Does this mean my Java version is too old?

[git]

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

​3852a0a

Further improve 3d graphics file handling

[jdemeyer]

I think this patch should fix the issues mentioned above (apart from Martin's comment which I decided to simply remove, note that doctests also generate random filenames).

[jdemeyer]

The error in 30 occurs with or without this patch, but I would still like to know what is wrong.

[jdemeyer]

With this patch, the filename= option does work in the notebook.

[kcrisman]

The error in 30 occurs with or without this patch, but I would still like to know what is wrong.

That indicates to me that your notebook instantiation is somehow corrupted and/or the display hook stuff didn't even come through. Earlier today I had it all working, for example, and now it doesn't even have the extra click boxes in the template, mysteriously. I'm baffled. But that shouldn't happen. Though you may be right about the old java.

[kcrisman]

cube(viewer="jmol")

The error in 30 occurs with or without this patch, but I would still like to know what is wrong.

That indicates to me that your notebook instantiation is somehow corrupted and/or the display hook stuff didn't even come through. Earlier today I had it all working, for example, and now it doesn't even have the extra click boxes in the template, mysteriously. I'm baffled. But that shouldn't happen. Though you may be right about the old java.

Yeah, I can't replicate this here, though the old java could possible contribute. What happens if you click on the "live 3d" box and then evaluate? That would bypass java creating the static image.

[jdemeyer]

Replying to kcrisman:

What happens if you click on the "live 3d" box and then evaluate?

When enabling "Load 3-D Live" I get the horrifying error message (it's Halloween, right?)

/usr/local/src/sage-git/local/lib/python2.7/site-packages/IPython/core/f\
ormatters.py:239: FormatterWarning: Exception in text/plain formatter:
Jmol failed to create file
'/tmp/tmpJEIc27/.jmol_images/sage0-size500.jmol.png', see
'/home/jdemeyer/.sage/temp/tamiyo/2874/tmp_IABUbj.txt' for details
  FormatterWarning,
None

then some 3d figure which does work nicely, then the sentence "Right-click to get options menu."

[jdemeyer]

When enabling "Use java for 3-D" and not "Load 3-D Live" I get a very similar result (with one interesting difference: the right-click menu uses a different font and is in English, with "Load 3-D Live" the menu was in Dutch).

(edit: cannot reproduce this anymore, getting a light grey square like in 40) (edit2: restarting the browser fixed this again)

[jdemeyer]

When enabling both "Use java for 3-D" and "Load 3-D Live" I get a result similar to 30 except that the blank square is light grey instead of red.

[jdemeyer]

For the record, I am using

java version "1.6.0_31"
OpenJDK Runtime Environment (IcedTea6 1.13.3) (Gentoo build 1.6.0_31-b31)
OpenJDK 64-Bit Server VM (build 23.25-b01, mixed mode)

and

Firefox ESR 24.8.0

[kcrisman]

I won't have time to look at this over the weekend, nor do I have the jmol/jsmol expertise to diagnose it, but I would recommend putting something about it on the 6.4.rc0 release thread so that Jonathan Gutow sees it.

[strogdon]

I don't know if anyone has tried it but I removed the patches associated with #17170 from 6.4.r0 and I still have the issue with viewer='tachyon' opening a jmol window instead of creating a png. So perhaps someother displayhook-related ticket?

[kcrisman]

I don't know if anyone has tried it but I removed the patches associated with #17170 from 6.4.r0 and I still have the issue with viewer='tachyon' opening a jmol window instead of creating a png. So perhaps someother displayhook-related ticket?

Hmm. Do you know where you first saw this? (I'm not suggesting you do a full bisection! Just wondering.)

[kcrisman]

I've tried to report the things on this ticket not actually related to this ticket to sage-release. However, now I've forgotten where we were in the review!

[gagern]

Replying to kcrisman:

I've forgotten where we were in the review!

The commit from comment:31 seems to deal with all outstanding issues from comment:19, so if you review that, I believe we should be fine. I guess Jeroen contributed enough that he should be an author as well. Do you want to file a ticket for the dynamic resizing of applets you mentioned in comment:19?

[strogdon]

Replying to kcrisman:

I don't know if anyone has tried it but I removed the patches associated with #17170 from 6.4.r0 and I still have the issue with viewer='tachyon' opening a jmol window instead of creating a png. So perhaps someother displayhook-related ticket?

Hmm. Do you know where you first saw this? (I'm not suggesting you do a full bisection! Just wondering.)

I believe on sage-release. But this is not related to this ticket. I only saw your comment:21 above. It should be addressed somewhere.

[jdemeyer]

Replying to kcrisman:

sage: var('y')
y
sage: plot3d(x^2+y^3,(x,-1,1),(y,-1,1),filename='foo')
sage: plot3d(x^2+y^3,(x,-1,1),(y,-1,1),filename='foo',viewer='tachyon')

Note that the second thing pops up a jmol viewer.

This is now #17284.

[jdemeyer]

For the record: I give positive_review to everything on this ticket not authored by me.

[kcrisman]

And I am happy with everything except I need to look at the very last patch one more time... okay, looks good other than two questions.

Question - is this a Python syntax I'm not aware of?

def show(self, *, filename=None, **kwds):

In particular, the extra * mystifies me. (Edit - okay, I found it, but it appears to be Python 3 only? I believe it works in recent Python 2.x since you used it, but I couldn't find a definitive reference - even the Python reference was ambiguous.)

Oh, and maybe add a doctest for ​this commit with the error raised? Maybe that's too trivial to do... but for some reason the doctest

sage: p.show(viewer='canvas3d')

doesn't raise an error, which makes sense because of the code. But it also is now doctested with the ​most recent change you made, despite what you just said earlier that making canvas3d guys was too long!

Somehow this should be resolved. I think the best way is to doctest the creation of the file, perhaps by setting a one-time flag or something, rather than the error - as long as we tell people that in the command line it will give an error. (Well, we could doctest both as well!)

[gagern]

Replying to kcrisman:

In particular, the extra * mystifies me. (Edit - okay, I found it, but it appears to be Python 3 only? I believe it works in recent Python 2.x since you used it, but I couldn't find a definitive reference - even the Python reference was ambiguous.)

This syntax for keyword-only parameters comes from Python 3 but is supported by Cython even for older versions of Python. In #16607 I suggested support for keyword-only parameters in pure Python code, and there jdemeyer pointed out that Cython support.

[kcrisman]

In particular, the extra * mystifies me. (Edit - okay, I found it, but it appears to be Python 3 only? I believe it works in recent Python 2.x since you used it, but I couldn't find a definitive reference - even the Python reference was ambiguous.)

This syntax for keyword-only parameters comes from Python 3 but is supported by Cython even for older versions of Python. In #16607 I suggested support for keyword-only parameters in pure Python code, and there jdemeyer pointed out that Cython support.

Aha.

With this patch, the filename= option does work in the notebook.

And it really does save to where needed, awesome! Especially in DATA could be useful.

despite what you just said earlier that making canvas3d guys was too long!

Okay, sorry, I see what you did now - got a little too excited there. What do you think about testing the warning?

Otherwise this was good teamwork by you two, very nice.

[jdemeyer]

Replying to kcrisman:

despite what you just said earlier that making canvas3d guys was too long!

Okay, sorry, I see what you did now - got a little too excited there. What do you think about testing the warning?

I'm too confused, what are you (plural) trying to say here?

[kcrisman]

Here is the short version:

p.show(viewer='canvas3d')

raises an error in the command line but not during doctesting because of the way you wrote the code (creates the file). Perhaps the warning should also be doctested.

[jdemeyer]

Replying to kcrisman:

Perhaps the warning should also be doctested.

That's possible but requires some messing around with DOCTEST_MODE. Given that it's a minor thing and that Volker has plans to change all this code anyway, I feel that it's not worth the trouble. But if you insist, I will add that test.

PS: I assume you mean "error", not "warning".

[kcrisman]

Perhaps the warning should also be doctested.

That's possible but requires some messing around with DOCTEST_MODE.

Yes.

Given that it's a minor thing and that Volker has plans to change all this code anyway, I feel that it's not worth the trouble. But if you insist, I will add that test.

Ok.

PS: I assume you mean "error", not "warning".

Yes, though I recall writing both of those a few times and then accidentally closing the browser/getting timed out!

[vbraun]

Conflicts with 6.4, please merge.

[git]

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

​e83f839

Merge tag '6.4' into ticket/16640

[jdemeyer]

Done, the conflict was the removal of a redundant import of tmp_filename.

[jdemeyer]

sage -t --long src/sage/repl/display/formatter.py
**********************************************************************
File "src/sage/repl/display/formatter.py", line 320, in sage.repl.display.formatter.SageNBTextFormatter.__call__
Failed example:
    fmt(FooGraphics())
Expected:
    showing graphics
    ''
Got:
    showing graphics
    doctest:239: FormatterWarning: Exception in text/plain formatter: [Errno 2] No such file or directory: '/nonexistent.png'
**********************************************************************

[git]

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

​034fa58

Fix doctest to use existing file