Pickling and otherwise enhancing global options

[AndrewMathas]

Instances of the GlobalOptions class in sage.structure.global_options do not pickle, which is annoying and causes various problems. (This is a defect.)

In addition, when the GlobalOptions class was introduced it was suggested in sage-dev that it "would be nice" if we could implement the syntax used in the IPython configuration:

    A.options.foobar = 1

in addition to

    A.options(foobar=1)

This ticket implements both of these features and, in addition, makes it possible to construct options classes dynamically.

As was also suggested on sage-dev, I have also renamed all of the global_options methods simply as options, with the global_options variants being deprecated. The "stand-alone" options classes such as PartitionOoptions, TableauxOptions, ... have also been deprecated and, instead, put inside their "parent" classes where they are accessible using the options method (global_options methods have been deprecated). This is cleaned up the code a little and it seems to speed up the sage start-up time.

The pickling is done by adding an extra module argument to the GlobalOptions that specifies the module which contains the class that the options are attached -- name of of the class defaults to the name of the options class but this can be explicitly set using the options_class argument. The options class is assumed to have an options method, and this is used to unpickle a pickle for an options class. GlobalOptions that do not arise as options methods for standard sage classes cannot be unpickled. The reason why the name of the module and class are passed, as strings, to GlobalOptions rather than the actual class is that this approach allows the options to be constructed during the initialisation of the (instance of the) class and, in turn, this allows the class to dynamically construct their options classes.

CC: @nathanncohen @tscrim

Component: interfaces

Keywords: days78, options

Author: Andrew Mathas

Branch/Commit: ba7dd98

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/18555

[AndrewMathas]

Changed keywords from none to options

[AndrewMathas]

Author: Andrew Mathas

[AndrewMathas]

comment:3

As I am cleaning this up a related question comes to mind: the name GlobalOptions is slightly unfortunate because instances of this class are really just options for an associated family of objects. I can sort of forgive myself for using this name for the class, but in all of the instances of this class the associated "parent" classes have a global_options method that points back to its options class. This method is now being enshrined more firmly inside the code because it is exploited to allow pickling. I would be happier if these methods were simply called options, rather than global_options, even though this change would necessitate deprecating the existing global_options methods.

Thoughts?

[AndrewMathas]

Branch: u/andrew.mathas/pickling_global_options

[AndrewMathas]

Commit: 2ba392f

[AndrewMathas]

New commits:

f006a84	Adding pickling methods
2ba392f	Initial version - some failing doctests

[sagetrac-git]

Changed commit from 2ba392f to 12cb846

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

3a4a9d2	20618: imported sage.modules.tutorial_free_modules and thematic_tutorials/tutorial-implementing-algebraic-structures from the Sage-Combinat queue
3e3e350	20618: fixed missing file
500d2d8	Global options to options
52d7cf6	Recasting options as attributes
64d0209	Global options as class attributes
25ab72f	Fixing doctests
23f2c56	Merging into 7.3.beta4
12cb846	Fixing more doc-tests

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

c676864	Global options to options
9313ce8	Recasting options as attributes
11a13f4	Global options as class attributes
217b5b4	Fixing doctests
b761fec	Merging into 7.3.beta4
caabdb0	Fixing more doc-tests
11b7080	More doc-tests

[sagetrac-git]

Changed commit from 12cb846 to 11b7080

[AndrewMathas]

comment:12

Replying to @sagetrac-git:

This was to remove 20618 from the commit tree as it shouldn't have been there...

[tscrim]

Changed branch from u/andrew.mathas/pickling_global_options to public/misc/pickling_global_options-18555

[tscrim]

comment:13

Okay, I fixed the doctest failures with permutation.py. The issue was that the _Option (now Option so it appears in the doc) did not define a __ne__ method, and so it returned True for things like opt != X even when opt == X.

The other doctest failures were due to lack of conversion to options from global_options.

This is a very nice improvement over the current interface to (global) options. If the doctest pass for you and my changes are good, then you can set this to a positive review.

---

New commits:

0636c87	Merge branch 'u/andrew.mathas/pickling_global_options' of trac.sagemath.org:sage into public/misc/pickling_global_options-18555
9642057	Making everything work with the new API.
f42beb4	Rename _Option to Option and some PEP8 to global_options.py.

[tscrim]

Changed commit from 11b7080 to f42beb4

[tscrim]

Changed keywords from options to days78, options

[sagetrac-git]

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

4013abb

Fixing failing doctests identified by the patchbot

[sagetrac-git]

Changed commit from f42beb4 to 4013abb

[tscrim]

comment:15

There's some doc issues in the manifold.py options: EXAMPLES:: and everything including and after sage: latex(f) is over indented.

[sagetrac-git]

Changed commit from 4013abb to 3f5e0e9

[vbraun]

comment:21

Reviewer name is missing

[tscrim]

Reviewer: Travis Scrimshaw

[vbraun]

comment:23

Merge conflict, try next beta

[sagetrac-git]

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

ed5070d

Merging 7.3.beta7

[sagetrac-git]

Changed commit from 8322fd5 to ed5070d

[AndrewMathas]

comment:25

Merged into 7.3.beta7. Tests pass.

[vbraun]

comment:26

Fails on 32-bit

sage -t --long src/sage/structure/global_options.py
**********************************************************************
File "src/sage/structure/global_options.py", line 599, in sage.structure.global_options.Option.__hash__
Failed example:
    hash( Tableaux.options.convention ) # indirect doc-test
Expected:
    -6673246059928475433
Got:
    -1691342633
**********************************************************************
1 item had failures:
   1 of   2 in sage.structure.global_options.Option.__hash__
    [135 tests, 1 failure, 0.03 s]

[tscrim]

comment:27

This just needs to mark the different output as # 32-bit and # 64-bit, but I'd change the actual test to hash(Tableaux.options.convention) == hash(Tableaux.options('convention') (it also doesn't need the # indirect doctest marker). Sorry for not catching that sooner.

[sagetrac-git]

Changed commit from ed5070d to ba7dd98

[sagetrac-git]

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

ba7dd98

Fixing hashed doct-test

[AndrewMathas]

comment:29

Replying to @tscrim:

This just needs to mark the different output as # 32-bit and # 64-bit, but I'd change the actual test to hash(Tableaux.options.convention) == hash(Tableaux.options('convention') (it also doesn't need the # indirect doctest marker). Sorry for not catching that sooner.

Thanks Travis! Fixed and changed back to a positive review.

[vbraun]

Changed branch from public/misc/pickling_global_options-18555 to ba7dd98