Opened 10 years ago

Closed 10 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:

Status badges

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

Download all attachments as: .zip

Change History (20)

comment:1 Changed 10 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 10 years ago by leif

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

comment:3 Changed 10 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 10 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 10 years ago by leif

  • Milestone changed from sage-4.7.2 to sage-4.7.1

comment:6 follow-up: Changed 10 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 10 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 10 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 10 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 10 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 10 years ago by jhpalmieri

for reference only: difference between old patch and new one

Changed 10 years ago by jhpalmieri

apply only this patch

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

for reference only: difference between old jhp patch and v2

Changed 10 years ago by jhpalmieri

apply only this patch

comment:14 Changed 10 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 10 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.