save(x,filename) fails for pure Python objects for x if filename contains a dot

[logix]

(The summary actually is not completely accurate - there might be some Python object this works for that I'm not aware of.)

If the filename passed to save() contains a dot, save() assumes that the user doesn't just want to dump the (pickled) object, but instead wants to call the object's save() method. I guess this makes sense in situations like save(g, 'mygraph.png'), but the code should fall back to dumping the pickled version (e.g. via try: ... except AttributeError: ... - suggested via IRC by leif) if the object has no save() method.

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 .wav file) and not statically compare with a list of known extensions.

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'

Here's the culprit:

    # Add '.sobj' if the filename currently has no extension
    if os.path.splitext(filename)[1] == '':
        filename += '.sobj'

    if filename.endswith('.sobj'):
        try:
            obj.save(filename=filename, compress=compress, **kwds)
        except (AttributeError, RuntimeError, TypeError):
            s = cPickle.dumps(obj, protocol=2)
            if compress:
                s = comp.compress(s)
            open(process(filename), 'wb').write(s)
    else:
        # Saving an object to an image file. 
        # XXX This of course fails for plain Python objects:
        obj.save(filename, **kwds)

---

Apply only trac_11577-jhp.v2.patch .

[leif]

What about simply adding .sobj above if there's no extension or the object has no save() method?

[leif]

Attached patch does exactly what I suggested last.

[jhpalmieri]

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.

[leif]

Replying to jhpalmieri:

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.

Ooops, I thought doctests would be executed there, i.e. sage{-doctest,doctest.py} would cd to that before running Python.

[leif]

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.

[leif]

Finally...

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 compress=True should be self-explanatory. For some reason I don't get the correct default parameters shown when typing save? at the Sage prompt though.)

I originally wanted to add

    import os.path
    from sage.misc.misc import SAGE_TMP
    filename = os.path.join(SAGE_TMP, "foo.bar")
    ...

but then saw the other examples, so I guess a lot of doctests rely on

• having SAGE_TMP already imported (by sage.all), and also
• SAGE_TMP having a trailing slash (or os.path.sep),

the latter being perhaps a bit more dangerous than the former.

So I also just used SAGE_TMP + "...".

[jhpalmieri]

At the sage: prompt, I get

Definition:       save(obj, filename, compress=None, **kwds=True)

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.

As far as SAGE_TMP is concerned, you're right, it's sloppy. Probably the right solution is to have a function sage_tmpfile or something like that, which takes one argument, FILE, and returns a valid path using os.path.join(SAGE_TMP, FILE). In fact, if you look at how SAGE_TMP is defined in the first place (in sage.misc.misc), it's defined using explicit slashes rather than with os.path.join. This all belongs on another ticket; if I have time, I'll work on that eventually.

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.

[jhpalmieri]

for reference only: difference between old patch and new one

[jhpalmieri]

apply only this patch

[leif]

Replying to jhpalmieri:

At the sage: prompt, I get

Definition:       save(obj, filename, compress=None, **kwds=True)

Oh, I did not notice the true keywords.

The most disturbing thing with SAGE_TMP is IMHO that it ignores settings of TMP, TMPDIR and TEMP, and -- worse -- does not even use /tmp/ which is more likely to be on a local (usually auto-cleaned) file system than $HOME/.sage/, and I don't expect people to change DOT_SAGE to live on a temporary filesystem. (Ok, one can still create a symbolic link from ~/.sage/temp to whatever is desired, but who knows? Especially since there's also ~/.sage/tmp/ and of course $SAGE_ROOT/tmp/.) We furthermore had SAGE_TMPDIR and SAGE_TESTDIR, the former meanwhile changed to the latter. I'll perhaps open a sage-devel thread on that (to make William happy).

I'm attaching a new version fixing one or two words in the docstring.

Ok. Regarding extensions, I'd consider foo.png having one, while foo.baz and foo.bar (non-exclusively) having some. :-)

[jdemeyer]

I have a doctest failure which might have been caused by this:

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

[jdemeyer]

Confirmed that this does indeed cause the above doctest failures on sage-4.7.1.

[jhpalmieri]

Here's a new patch, along with a "delta" patch.

[jhpalmieri]

for reference only: difference between old jhp patch and v2

[jhpalmieri]

apply only this patch

[leif]

New patch looks reasonable, and passes all [long] tests in sage/{misc,structure}/, so positive review. (Tested with Sage 4.7.1.rc2.)