Opened 8 years ago

Closed 8 years ago

#11577 closed defect (fixed)

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

Reported by: logix Owned by: was
Priority: minor Milestone: sage-4.7.2
Component: pickling Keywords: .sobj
Cc: Merged in: sage-4.7.2.alpha2
Authors: Leif Leonhardy, John Palmieri Reviewers: John Palmieri, Leif Leonhardy
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by leif)

(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.

Attachments (5)

trac_11577-fix_save_to_filenames_with_dots.patch (2.8 KB) - added by leif 8 years ago.
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.
trac_11577-delta.patch (1001 bytes) - added by jhpalmieri 8 years ago.
for reference only: difference between old patch and new one
trac_11577-jhp.patch (2.9 KB) - added by jhpalmieri 8 years ago.
apply only this patch
trac_11577-delta-1to2.patch (694 bytes) - added by jhpalmieri 8 years ago.
for reference only: difference between old jhp patch and v2
trac_11577-jhp.v2.patch (3.0 KB) - added by jhpalmieri 8 years ago.
apply only this patch

Download all attachments as: .zip

Change History (20)

comment:1 Changed 8 years ago by logix

  • Description modified (diff)
  • Summary changed from save(x,filename) fails for some types of objects for x if filename contains a dot to save(x,filename) fails for pure Python objects for x if filename contains a dot

comment:2 Changed 8 years ago by leif

  • Component changed from sage-mode to pickling
  • Description modified (diff)
  • Owner changed from ncalexan to was

comment:3 Changed 8 years ago by leif

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

comment:4 Changed 8 years ago by leif

  • Authors set to Leif Leonhardy
  • Status changed from new to needs_review

Attached patch does exactly what I suggested last.

comment:5 Changed 8 years ago by leif

  • Milestone changed from sage-4.7.2 to sage-4.7.1

comment:6 follow-up: Changed 8 years ago by jhpalmieri

  • Status changed from needs_review to needs_work

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.

comment:7 in reply to: ↑ 6 Changed 8 years ago by 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.

Changed 8 years ago by 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.

comment:8 Changed 8 years ago by leif

  • Status changed from needs_work to needs_review

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 + "...".

comment:9 follow-up: Changed 8 years ago by jhpalmieri

  • Description modified (diff)
  • Milestone changed from sage-4.7.1 to sage-4.7.2
  • Reviewers set to John Palmieri
  • Status changed from needs_review to positive_review

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.

Changed 8 years ago by jhpalmieri

for reference only: difference between old patch and new one

Changed 8 years ago by jhpalmieri

apply only this patch

comment:10 in reply to: ↑ 9 Changed 8 years ago by 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. :-)

comment:11 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

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

comment:12 Changed 8 years ago by jdemeyer

  • Work issues set to sage/misc/cachefunc.py doctest failure

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

comment:13 Changed 8 years ago by jhpalmieri

  • Description modified (diff)
  • Status changed from needs_work to needs_review

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

Changed 8 years ago by jhpalmieri

for reference only: difference between old jhp patch and v2

Changed 8 years ago by jhpalmieri

apply only this patch

comment:14 Changed 8 years ago by leif

  • Authors changed from Leif Leonhardy to Leif Leonhardy, John Palmieri
  • Description modified (diff)
  • Keywords .sobj added
  • Reviewers changed from John Palmieri to John Palmieri, Leif Leonhardy
  • Status changed from needs_review to positive_review
  • Work issues sage/misc/cachefunc.py doctest failure deleted

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

comment:15 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.7.2.alpha2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.