Opened 10 years ago

Closed 10 years ago

#11069 closed enhancement (fixed)

Don't use version of Sage in default pickle directory

Reported by: jdemeyer Owned by: was
Priority: major Milestone: sage-4.7.1
Component: pickling Keywords: pickle
Cc: Merged in: sage-4.7.1.alpha2
Authors: Jeroen Demeyer Reviewers: Nicolas M. Thiéry
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

I don't see any good reason why the version number of Sage is used when creating pickles using sage.structure.sage_object.picklejar

def picklejar(obj, dir=None):
    """
    Create pickled sobj of obj in dir, with name the absolute value of
    the hash of the pickle of obj.  This is used in conjunction with
    sage.structure.sage_object.unpickle_all.

    To use this to test the whole Sage library right now, set the
    environment variable SAGE_PICKLE_JAR, which will make it so dumps
    will by default call picklejar with the default dir.  Once you do
    that and doctest Sage, you'll find that the SAGE_ROOT/tmp/
    contains a bunch of pickled objects along with corresponding txt
    descriptions of them.  Use the
    sage.structure.sage_object.unpickle_all to see if they unpickle
    later.

    INPUTS:

    - ``obj`` - a pickleable object

    - ``dir`` - a string or None; if None defaults to
      ``SAGE_ROOT/tmp/pickle_jar-version``

See #10768 for a potentially conflicting ticket.

Apply:

  1. 11069_picklejar.patch (positive_review)
  2. 11069_restore_permissions.patch (needs_review)

Attachments (2)

11069_picklejar.patch (1.9 KB) - added by jdemeyer 10 years ago.
11069_restore_permissions.patch (865 bytes) - added by jdemeyer 10 years ago.

Download all attachments as: .zip

Change History (13)

Changed 10 years ago by jdemeyer

comment:1 Changed 10 years ago by jdemeyer

  • Status changed from new to needs_review

comment:2 Changed 10 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 10 years ago by nthiery

  • Status changed from needs_review to positive_review

I myself don't have a strong feeling one way or the other for including the version. Since the release manager is the main user of this function, I happy to abide if he finds it more practical. Beside this harmless change, the patch adds more tests and fixes a typo. And all test pass. So, positive review.

comment:4 Changed 10 years ago by nthiery

  • Reviewers set to Nicolas M. Thiéry

Changed 10 years ago by jdemeyer

comment:5 Changed 10 years ago by jdemeyer

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

comment:6 Changed 10 years ago by jdemeyer

  • Status changed from needs_work to needs_review

Additional patch needs review.

comment:7 follow-up: Changed 10 years ago by nthiery

I don't know if it matters in any way, but is there an easy way to restore the original permissions, whatether they are, rather than forcing to 755?

comment:8 in reply to: ↑ 7 Changed 10 years ago by jdemeyer

Replying to nthiery:

I don't know if it matters in any way, but is there an easy way to restore the original permissions, whatether they are, rather than forcing to 755?

I guess one could use os.stat() but I don't see a reason to add this complication. The only thing which matters is that the directory can be removed without errors.

comment:9 Changed 10 years ago by nthiery

Ok, good enough. Please check the failure reported by the buildbot. If it's just a random independent thing, then you can set a positive review on my behalf.

Cheers,

comment:10 Changed 10 years ago by jdemeyer

  • Status changed from needs_review to positive_review

It is a random independent thing:

The following tests failed:

	sage -t  -force_lib devel/sage-11069/sage/schemes/plane_conics/all.py # 0 doctests failed

comment:11 Changed 10 years ago by jdemeyer

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