Opened 8 years ago

Last modified 8 years ago

#11577 closed defect

save(x,filename) fails for pure Python objects for x if filename contains a dot — at Version 9

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

Description (last modified by jhpalmieri)

(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 trac_11577-jhp.patch.

Change History (12)

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

Note: See TracTickets for help on using tickets.