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: |
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 10 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 10 years ago by
- Component changed from sage-mode to pickling
- Description modified (diff)
- Owner changed from ncalexan to was
comment:3 Changed 10 years ago by
comment:4 Changed 10 years ago by
- Status changed from new to needs_review
Attached patch does exactly what I suggested last.
comment:5 Changed 10 years ago by
- Milestone changed from sage-4.7.2 to sage-4.7.1
comment:6 follow-up: ↓ 7 Changed 10 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 10 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 10 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 10 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 10 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 10 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 10 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 10 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 10 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 10 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 10 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?