Opened 7 months ago

Closed 6 months ago

#16349 closed defect (fixed)

Make UniqueFactory unpickling more flexible

Reported by: tscrim Owned by: tscrim
Priority: major Milestone: sage-6.3
Component: pickling Keywords: UniqueFactory pickle
Cc: SimonKing, nthiery Merged in:
Authors: Simon King, Travis Scrimshaw Reviewers: Travis Scrimshaw, Simon King
Report Upstream: N/A Work issues:
Branch: c7646c1 (Commits) Commit: c7646c1a28493f94a5362212cf8bd089ffce3279
Dependencies: Stopgaps:

Description

Currently UniqueFactory's unpickling protocol is to call generic_factory_unpickle(), whose first argument must be an instance of UniqueFactory. However we might want to change the object in the global namespace, let's say to a function, and then we will not be able to unpickle any thing beforehand (register_unpickle_override can not help here because the pickle info is (UniqueFactory, generic_factory_unpickle)). Came up in #15289.

Change History (9)

comment:1 Changed 7 months ago by SimonKing

  • Branch set to u/SimonKing/ticket/16349
  • Created changed from 05/13/14 08:42:28 to 05/13/14 08:42:28
  • Modified changed from 05/13/14 08:42:28 to 05/13/14 08:42:28

comment:2 Changed 7 months ago by SimonKing

  • Authors changed from Travis Scrimshaw to Simon King
  • Commit set to 321a9e407ef260269f4d66159a787316440082e3
  • Status changed from new to needs_review

Travis, you have been inserted as "Author", but I think you did not provide code. So, I took the liberty to replace your name by mine, and attach a branch. The new doctest demonstrates how to replace a unique factory by unique representation and correctly unpickle.


New commits:

321a9e4Unpickling when replacing an old UniqueFactory by something new

comment:3 Changed 7 months ago by git

  • Commit changed from 321a9e407ef260269f4d66159a787316440082e3 to 187d1aa8d95cf2b64ab2c45224844a709412127b

Branch pushed to git repo; I updated commit sha1. New commits:

187d1aaFix arguments passed to the constructor that replaces an old UniqueFactory

comment:4 Changed 7 months ago by SimonKing

I had to fix one detail (I made a wrong assumption on the format of _factory_data.

comment:5 Changed 7 months ago by tscrim

Hey Simon, I didn't have a chance to finish it yesterday. I will finish up what I was working on today as an alternative proposal.

comment:6 Changed 7 months ago by tscrim

  • Authors changed from Simon King to Simon King, Travis Scrimshaw
  • Branch changed from u/SimonKing/ticket/16349 to public/pickling/unique_factories-16349
  • Commit changed from 187d1aa8d95cf2b64ab2c45224844a709412127b to c7646c1a28493f94a5362212cf8bd089ffce3279
  • Reviewers set to Travis Scrimshaw, Simon King

Okay I've just put in both versions. I've implemented something similar to register_unpickle_override() (which I've called register_factory_unpickle()). This way we can handle when the factory is removed from the global namespace (such as for name change). If you could check that and agree, then we can set this to positive review. Thanks.


New commits:

83c79efMerge branch 'u/SimonKing/ticket/16349' of trac.sagemath.org:sage into public/pickling/unique_factories-16349
2df09edImplemented an unpickle override for UniqueFactory.
c7646c1Fixed doctest caused by me forgetting the registration is global.

comment:7 Changed 6 months ago by SimonKing

  • Status changed from needs_review to positive_review

With the current branch, we provide two ways to deal with old pickles of UniqueFactory: If we replace the old factory by something new that has the same name, is in the same module and can process the same input as the UniqueFactory, then nothing more needs to be done (that's my contribution). Moreover, it is possible to override unpickling so that a new callable is used for unpickling even if the old factory is still there (that's your contribution). I think both possibilities make sense. Hence, I complete the positive review.

comment:8 Changed 6 months ago by tscrim

Thank you Simon.

comment:9 Changed 6 months ago by vbraun

  • Branch changed from public/pickling/unique_factories-16349 to c7646c1a28493f94a5362212cf8bd089ffce3279
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.