Sage: Ticket #11577: save(x,filename) fails for pure Python objects for x if filename contains a dot
https://trac.sagemath.org/ticket/11577
<p>
(The summary actually is not completely accurate - there might be some Python object this works for that I'm not aware of.)
</p>
<p>
If the filename passed to <code>save()</code> contains a dot, <code>save()</code> assumes that the user doesn't just want to dump the (pickled) object, but instead wants to call the object's <code>save()</code> method. I guess this makes sense in situations like <code>save(g, 'mygraph.png')</code>, but the code should fall back to dumping the pickled version (e.g. via <code>try: ... except AttributeError: ...</code> - suggested via IRC by leif) if the object has no <code>save()</code> method.
</p>
<p>
leif also suggested checking if the file name extension is known - however I guess that we then should verify this with the object itself (e.g. it wouldn't make sense to save a graphics object to a <code>.wav</code> file) and not statically compare with a list of known extensions.
</p>
<pre class="wiki">sage: save((1,1), 'foo2')
sage: save(Matrix(3,3), 'foo.bar3')
sage: save((1,1), 'foo.bar4')
---------------------------------------------------------------------------
AttributeError Traceback (most recent call last)
/tmp/sagedebug/<ipython console> in <module>()
/usr/local/sage/local/lib/python2.6/site-packages/sage/structure/sage_object.so in sage.structure.sage_object.save (sage/structure/sage_object.c:8156)()
AttributeError: 'tuple' object has no attribute 'save'
</pre><p>
Here's the culprit:
</p>
<div class="wiki-code"><div class="code"><pre> <span class="c"># Add '.sobj' if the filename currently has no extension</span>
<span class="k">if</span> os<span class="o">.</span>path<span class="o">.</span>splitext<span class="p">(</span>filename<span class="p">)[</span><span class="mi">1</span><span class="p">]</span> <span class="o">==</span> <span class="s">''</span><span class="p">:</span>
filename <span class="o">+=</span> <span class="s">'.sobj'</span>
<span class="k">if</span> filename<span class="o">.</span>endswith<span class="p">(</span><span class="s">'.sobj'</span><span class="p">):</span>
<span class="k">try</span><span class="p">:</span>
obj<span class="o">.</span>save<span class="p">(</span>filename<span class="o">=</span>filename<span class="p">,</span> compress<span class="o">=</span>compress<span class="p">,</span> <span class="o">**</span>kwds<span class="p">)</span>
<span class="k">except</span> <span class="p">(</span><span class="ne">AttributeError</span><span class="p">,</span> <span class="ne">RuntimeError</span><span class="p">,</span> <span class="ne">TypeError</span><span class="p">):</span>
s <span class="o">=</span> cPickle<span class="o">.</span>dumps<span class="p">(</span>obj<span class="p">,</span> protocol<span class="o">=</span><span class="mi">2</span><span class="p">)</span>
<span class="k">if</span> compress<span class="p">:</span>
s <span class="o">=</span> comp<span class="o">.</span>compress<span class="p">(</span>s<span class="p">)</span>
<span class="nb">open</span><span class="p">(</span>process<span class="p">(</span>filename<span class="p">),</span> <span class="s">'wb'</span><span class="p">)</span><span class="o">.</span>write<span class="p">(</span>s<span class="p">)</span>
<span class="k">else</span><span class="p">:</span>
<span class="c"># Saving an object to an image file. </span>
<span class="c"># XXX This of course fails for plain Python objects:</span>
obj<span class="o">.</span>save<span class="p">(</span>filename<span class="p">,</span> <span class="o">**</span>kwds<span class="p">)</span>
</pre></div></div><hr />
<p>
Apply only <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/11577/trac_11577-jhp.v2.patch" title="Attachment 'trac_11577-jhp.v2.patch' in Ticket #11577">trac_11577-jhp.v2.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/11577/trac_11577-jhp.v2.patch" title="Download"></a>.
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/11577
Trac 1.1.6logixTue, 05 Jul 2011 15:01:25 GMTdescription, summary changed
https://trac.sagemath.org/ticket/11577#comment:1
https://trac.sagemath.org/ticket/11577#comment:1
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/11577?action=diff&version=1">diff</a>)
</li>
<li><strong>summary</strong>
changed from <em>save(x,filename) fails for some types of objects for x if filename contains a dot</em> to <em>save(x,filename) fails for pure Python objects for x if filename contains a dot</em>
</li>
</ul>
TicketleifTue, 05 Jul 2011 15:13:39 GMTowner, component, description changed
https://trac.sagemath.org/ticket/11577#comment:2
https://trac.sagemath.org/ticket/11577#comment:2
<ul>
<li><strong>owner</strong>
changed from <em>ncalexan</em> to <em>was</em>
</li>
<li><strong>component</strong>
changed from <em>sage-mode</em> to <em>pickling</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/11577?action=diff&version=2">diff</a>)
</li>
</ul>
TicketleifTue, 05 Jul 2011 15:19:46 GMT
https://trac.sagemath.org/ticket/11577#comment:3
https://trac.sagemath.org/ticket/11577#comment:3
<p>
What about simply adding <code>.sobj</code> above if there's no extension <strong>or</strong> the object has no <code>save()</code> method?
</p>
TicketleifTue, 05 Jul 2011 16:45:19 GMTstatus changed; author set
https://trac.sagemath.org/ticket/11577#comment:4
https://trac.sagemath.org/ticket/11577#comment:4
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
<li><strong>author</strong>
set to <em>Leif Leonhardy</em>
</li>
</ul>
<p>
Attached patch does exactly what I suggested last.
</p>
TicketleifTue, 05 Jul 2011 16:46:59 GMTmilestone changed
https://trac.sagemath.org/ticket/11577#comment:5
https://trac.sagemath.org/ticket/11577#comment:5
<ul>
<li><strong>milestone</strong>
changed from <em>sage-4.7.2</em> to <em>sage-4.7.1</em>
</li>
</ul>
TicketjhpalmieriSat, 23 Jul 2011 19:34:27 GMTstatus changed
https://trac.sagemath.org/ticket/11577#comment:6
https://trac.sagemath.org/ticket/11577#comment:6
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
You shouldn't write to a file like "foo.bar" in a doctest. Instead, make sure you write to a temporary directory, for example using SAGE_TMP.
</p>
TicketleifSat, 23 Jul 2011 19:52:00 GMT
https://trac.sagemath.org/ticket/11577#comment:7
https://trac.sagemath.org/ticket/11577#comment:7
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11577#comment:6" title="Comment 6">jhpalmieri</a>:
</p>
<blockquote class="citation">
<p>
You shouldn't write to a file like "foo.bar" in a doctest. Instead, make sure you write to a temporary directory, for example using SAGE_TMP.
</p>
</blockquote>
<p>
Ooops, I thought doctests would be executed there, i.e. <code>sage{-doctest,doctest.py}</code> would <code>cd</code> to that before running Python.
</p>
TicketleifSat, 23 Jul 2011 22:47:08 GMTattachment set
https://trac.sagemath.org/ticket/11577
https://trac.sagemath.org/ticket/11577
<ul>
<li><strong>attachment</strong>
set to <em>trac_11577-fix_save_to_filenames_with_dots.patch</em>
</li>
</ul>
<p>
Sage library patch. Adds ".sobj" to filename if the object has no save() method. (Corrected and improved version.) Based on Sage 4.7.1.rc0.
</p>
TicketleifSat, 23 Jul 2011 23:36:06 GMTstatus changed
https://trac.sagemath.org/ticket/11577#comment:8
https://trac.sagemath.org/ticket/11577#comment:8
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
Finally...
</p>
<p>
The updated patch now adds another doctest and clarifies the description in the docstring. (I intentionally haven't changed it to enumerate all parameters as we normally do, as I think it is more readable as it is now, and <code>compress=True</code> should be self-explanatory. For some reason I don't get the correct default parameters shown when typing <code>save?</code> at the Sage prompt though.)
</p>
<p>
I originally wanted to add
</p>
<div class="wiki-code"><div class="code"><pre> <span class="kn">import</span> <span class="nn">os.path</span>
<span class="kn">from</span> <span class="nn">sage.misc.misc</span> <span class="kn">import</span> SAGE_TMP
filename <span class="o">=</span> os<span class="o">.</span>path<span class="o">.</span>join<span class="p">(</span>SAGE_TMP<span class="p">,</span> <span class="s">"foo.bar"</span><span class="p">)</span>
<span class="o">...</span>
</pre></div></div><p>
but then saw the other examples, so I guess <em>a lot of</em> doctests rely on
</p>
<ul><li>having <code>SAGE_TMP</code> already imported (by <code>sage.all</code>), and also
</li><li><code>SAGE_TMP</code> having a trailing slash (or <code>os.path.sep</code>),
</li></ul><p>
the latter being perhaps a bit more dangerous than the former.
</p>
<p>
So I also just used <code>SAGE_TMP + "..."</code>.
</p>
TicketjhpalmieriSun, 24 Jul 2011 03:23:27 GMTstatus, description, milestone changed; reviewer set
https://trac.sagemath.org/ticket/11577#comment:9
https://trac.sagemath.org/ticket/11577#comment:9
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>reviewer</strong>
set to <em>John Palmieri</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/11577?action=diff&version=9">diff</a>)
</li>
<li><strong>milestone</strong>
changed from <em>sage-4.7.1</em> to <em>sage-4.7.2</em>
</li>
</ul>
<p>
At the sage: prompt, I get
</p>
<pre class="wiki">Definition: save(obj, filename, compress=None, **kwds=True)
</pre><p>
Odd, the values are shifted right by one (filename should be None, compress should be True). I wonder what's causing that. Other functions in the same file seem to be okay. I have some old copies of Sage around, and this behavior dates back to at least version 4.3.
</p>
<p>
As far as <code>SAGE_TMP</code> is concerned, you're right, it's sloppy. Probably the right solution is to have a function <code>sage_tmpfile</code> or something like that, which takes one argument, FILE, and returns a valid path using <code>os.path.join(SAGE_TMP, FILE)</code>. In fact, if you look at how <code>SAGE_TMP</code> is defined in the first place (in sage.misc.misc), it's defined using explicit slashes rather than with <code>os.path.join</code>. This all belongs on another ticket; if I have time, I'll work on that eventually.
</p>
<p>
I'm happy with the patch, basically as is. I'm attaching a new version fixing one or two words in the docstring. I'm marking it as "positive review"; if you don't like my changes, switch it back.
</p>
TicketjhpalmieriSun, 24 Jul 2011 03:23:56 GMTattachment set
https://trac.sagemath.org/ticket/11577
https://trac.sagemath.org/ticket/11577
<ul>
<li><strong>attachment</strong>
set to <em>trac_11577-delta.patch</em>
</li>
</ul>
<p>
for reference only: difference between old patch and new one
</p>
TicketjhpalmieriSun, 24 Jul 2011 03:24:08 GMTattachment set
https://trac.sagemath.org/ticket/11577
https://trac.sagemath.org/ticket/11577
<ul>
<li><strong>attachment</strong>
set to <em>trac_11577-jhp.patch</em>
</li>
</ul>
<p>
apply only this patch
</p>
TicketleifSun, 24 Jul 2011 06:46:35 GMT
https://trac.sagemath.org/ticket/11577#comment:10
https://trac.sagemath.org/ticket/11577#comment:10
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11577#comment:9" title="Comment 9">jhpalmieri</a>:
</p>
<blockquote class="citation">
<p>
At the sage: prompt, I get
</p>
</blockquote>
<pre class="wiki">Definition: save(obj, filename, compress=None, **kwds=True)
</pre><p>
Oh, I did not notice the true keywords.
</p>
<p>
The most disturbing thing with <code>SAGE_TMP</code> is IMHO that it ignores settings of <code>TMP</code>, <code>TMPDIR</code> and <code>TEMP</code>, and -- worse -- does not even use <code>/tmp/</code> which is more likely to be on a local (usually auto-cleaned) file system than <code>$HOME/.sage/</code>, and I don't expect people to change <code>DOT_SAGE</code> to live on a temporary filesystem. (Ok, one can still create a symbolic link from <code>~/.sage/temp</code> to whatever is desired, but who knows? Especially since there's also <code>~/.sage/tmp/</code> and of course <code>$SAGE_ROOT/tmp/</code>.) We furthermore had <code>SAGE_TMPDIR</code> and <code>SAGE_TESTDIR</code>, the former meanwhile changed to the latter. I'll perhaps open a <code>sage-devel</code> thread on that (to make William happy).
</p>
<blockquote class="citation">
<p>
I'm attaching a new version fixing one or two words in the docstring.
</p>
</blockquote>
<p>
Ok. Regarding extensions, I'd consider <code>foo.png</code> having <em>one</em>, while <code>foo.baz</code> and <code>foo.bar</code> (non-exclusively) having <em>some</em>. :-)
</p>
TicketjdemeyerThu, 18 Aug 2011 06:18:00 GMTstatus changed
https://trac.sagemath.org/ticket/11577#comment:11
https://trac.sagemath.org/ticket/11577#comment:11
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
I have a doctest failure which might have been caused by this:
</p>
<pre class="wiki">sage -t -force_lib devel/sage/sage/misc/cachefunc.py
**********************************************************************
File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.7.2.alpha2/devel/sage-main/sage/misc/cachefunc.py", line 1106:
sage: for f in sorted(FC.file_list()): print f[len(dir):]
Expected:
/t-.key.sobj
/t-.sobj
/t-1_2.key.sobj
/t-1_2.sobj
/t-a-1.1.key.sobj
/t-a-1.1.sobj
Got:
/t-.key.sobj.sobj
/t-.sobj.sobj
/t-1_2.key.sobj.sobj
/t-1_2.sobj.sobj
/t-a-1.1.key.sobj.sobj
/t-a-1.1.sobj.sobj
</pre>
TicketjdemeyerThu, 18 Aug 2011 09:33:00 GMTwork_issues set
https://trac.sagemath.org/ticket/11577#comment:12
https://trac.sagemath.org/ticket/11577#comment:12
<ul>
<li><strong>work_issues</strong>
set to <em>sage/misc/cachefunc.py doctest failure</em>
</li>
</ul>
<p>
Confirmed that this does indeed cause the above doctest failures on sage-4.7.1.
</p>
TicketjhpalmieriThu, 18 Aug 2011 22:59:36 GMTstatus, description changed
https://trac.sagemath.org/ticket/11577#comment:13
https://trac.sagemath.org/ticket/11577#comment:13
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/11577?action=diff&version=13">diff</a>)
</li>
</ul>
<p>
Here's a new patch, along with a "delta" patch.
</p>
TicketjhpalmieriThu, 18 Aug 2011 23:00:05 GMTattachment set
https://trac.sagemath.org/ticket/11577
https://trac.sagemath.org/ticket/11577
<ul>
<li><strong>attachment</strong>
set to <em>trac_11577-delta-1to2.patch</em>
</li>
</ul>
<p>
for reference only: difference between old jhp patch and v2
</p>
TicketjhpalmieriThu, 18 Aug 2011 23:00:18 GMTattachment set
https://trac.sagemath.org/ticket/11577
https://trac.sagemath.org/ticket/11577
<ul>
<li><strong>attachment</strong>
set to <em>trac_11577-jhp.v2.patch</em>
</li>
</ul>
<p>
apply only this patch
</p>
TicketleifFri, 19 Aug 2011 01:21:28 GMTstatus, author, reviewer, description changed; keywords set; work_issues deleted
https://trac.sagemath.org/ticket/11577#comment:14
https://trac.sagemath.org/ticket/11577#comment:14
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>work_issues</strong>
<em>sage/misc/cachefunc.py doctest failure</em> deleted
</li>
<li><strong>author</strong>
changed from <em>Leif Leonhardy</em> to <em>Leif Leonhardy, John Palmieri</em>
</li>
<li><strong>keywords</strong>
<em>.sobj</em> added
</li>
<li><strong>reviewer</strong>
changed from <em>John Palmieri</em> to <em>John Palmieri, Leif Leonhardy</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/11577?action=diff&version=14">diff</a>)
</li>
</ul>
<p>
New patch looks reasonable, and passes all [long] tests in <code>sage/{misc,structure}/</code>, so <strong>positive review</strong>. (Tested with Sage 4.7.1.rc2.)
</p>
TicketjdemeyerMon, 22 Aug 2011 17:34:16 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/11577#comment:15
https://trac.sagemath.org/ticket/11577#comment:15
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-4.7.2.alpha2</em>
</li>
</ul>
Ticket