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 )
(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)
Change History (20)
comment:1 Changed 8 years ago by
- 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
- Component changed from sage-mode to pickling
- Description modified (diff)
- Owner changed from ncalexan to was
comment:3 Changed 8 years ago by
comment:4 Changed 8 years ago by
- Status changed from new to needs_review
Attached patch does exactly what I suggested last.
comment:5 Changed 8 years ago by
- Milestone changed from sage-4.7.2 to sage-4.7.1
comment:6 follow-up: ↓ 7 Changed 8 years ago by
- 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
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
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
- 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 (bysage.all
), and also SAGE_TMP
having a trailing slash (oros.path.sep
),
the latter being perhaps a bit more dangerous than the former.
So I also just used SAGE_TMP + "..."
.
comment:9 follow-up: ↓ 10 Changed 8 years ago by
- 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.
comment:10 in reply to: ↑ 9 Changed 8 years ago by
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
- 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
- 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
- Description modified (diff)
- Status changed from needs_work to needs_review
Here's a new patch, along with a "delta" patch.
comment:14 Changed 8 years ago by
- 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
- Merged in set to sage-4.7.2.alpha2
- Resolution set to fixed
- Status changed from positive_review to closed
What about simply adding
.sobj
above if there's no extension or the object has nosave()
method?