Opened 5 years ago

Closed 3 years ago

#18555 closed enhancement (fixed)

Pickling and otherwise enhancing global options

Reported by: andrew.mathas Owned by:
Priority: major Milestone: sage-7.3
Component: interfaces Keywords: days78, options
Cc: ncohen, tscrim Merged in:
Authors: Andrew Mathas Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: ba7dd98 (Commits) Commit: ba7dd9836386ed119c257d9bb42030149e091b47
Dependencies: Stopgaps:

Description (last modified by andrew.mathas)

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.

Change History (30)

comment:1 Changed 5 years ago by ncohen

  • Cc ncohen added

comment:2 Changed 5 years ago by andrew.mathas

  • Authors set to Andrew Mathas
  • Component changed from PLEASE CHANGE to interfaces
  • Description modified (diff)
  • Keywords options added
  • Summary changed from Pickling global options to Pickling and otherwise enhancing global options
  • Type changed from PLEASE CHANGE to enhancement

comment:3 Changed 5 years ago by andrew.mathas

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?

Last edited 3 years ago by andrew.mathas (previous) (diff)

comment:4 Changed 5 years ago by andrew.mathas

  • Description modified (diff)

comment:5 Changed 5 years ago by andrew.mathas

  • Cc tscrim added

comment:6 Changed 5 years ago by andrew.mathas

  • Branch set to u/andrew.mathas/pickling_global_options

comment:7 Changed 5 years ago by andrew.mathas

  • Commit set to 2ba392f42e9ee4b362cf61e8ddd2a9d419f0787e
  • Description modified (diff)

New commits:

f006a84Adding pickling methods
2ba392fInitial version - some failing doctests

comment:8 Changed 5 years ago by andrew.mathas

  • Description modified (diff)

comment:9 Changed 3 years ago by git

  • Commit changed from 2ba392f42e9ee4b362cf61e8ddd2a9d419f0787e to 12cb84666d10cab49c8d8b21aad73485c2fedaef

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

3a4a9d220618: imported sage.modules.tutorial_free_modules and thematic_tutorials/tutorial-implementing-algebraic-structures from the Sage-Combinat queue
3e3e35020618: fixed missing file
500d2d8Global options to options
52d7cf6Recasting options as attributes
64d0209Global options as class attributes
25ab72fFixing doctests
23f2c56Merging into 7.3.beta4
12cb846Fixing more doc-tests

comment:10 Changed 3 years ago by andrew.mathas

  • Description modified (diff)

comment:11 follow-up: Changed 3 years ago by git

  • Commit changed from 12cb84666d10cab49c8d8b21aad73485c2fedaef to 11b7080fb16f62f64d39bca2286d0912c55a2125

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

c676864Global options to options
9313ce8Recasting options as attributes
11a13f4Global options as class attributes
217b5b4Fixing doctests
b761fecMerging into 7.3.beta4
caabdb0Fixing more doc-tests
11b7080More doc-tests

comment:12 in reply to: ↑ 11 Changed 3 years ago by andrew.mathas

Replying to git:

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

Last edited 3 years ago by andrew.mathas (previous) (diff)

comment:13 Changed 3 years ago by tscrim

  • Branch changed from u/andrew.mathas/pickling_global_options to public/misc/pickling_global_options-18555
  • Commit changed from 11b7080fb16f62f64d39bca2286d0912c55a2125 to f42beb4a800cf4fd9c07acfbd054fd91b896edb2
  • Keywords days78 added
  • Status changed from new to needs_review

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:

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

comment:14 Changed 3 years ago by git

  • Commit changed from f42beb4a800cf4fd9c07acfbd054fd91b896edb2 to 4013abb4bbea7687da60f3240127027475d5e410

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

4013abbFixing failing doctests identified by the patchbot

comment:15 Changed 3 years ago by tscrim

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

comment:16 Changed 3 years ago by git

  • Commit changed from 4013abb4bbea7687da60f3240127027475d5e410 to 3f5e0e91e2289dcee18ad56b30c6884f9d26c890

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

3f5e0e9Fixing manifold documemtation issues and collecting all deprecation messages at the end of the files

comment:17 Changed 3 years ago by andrew.mathas

  • Description modified (diff)

Thanks Travis. Fixed. I have also cleaned up the deprecation messages, putting them in the same place, so that they will be easier to remove in future.

Finally, should the reset method be _reset() or Reset()? Currently it is _reset() but it probably shouldn't be hidden.

comment:18 Changed 3 years ago by tscrim

  • Milestone changed from sage-6.8 to sage-7.3

Reset() breaks python conventions, so that is a -1 from me. I think reset() or _reset() are both fine. Yet there is not so much a good general reason to go back to defaults other than for testing purposes in my mind. Hence the hidden _reset() is better IMO.

One last thing, you added an __init__ method in interval_poset.py. Is this necessary? If so, it needs a doctest. Once this is done, this will be at a positive review.

comment:19 Changed 3 years ago by git

  • Commit changed from 3f5e0e91e2289dcee18ad56b30c6884f9d26c890 to 8322fd5fe18b1bb624b78fbeccf2a8c37bd5e69f

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

8322fd5Removing __init__ from interval_poset.py

comment:20 Changed 3 years ago by andrew.mathas

  • Status changed from needs_review to positive_review

Fixed -- not sure how that got in there. Thanks Travis!

comment:21 Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work

Reviewer name is missing

comment:22 Changed 3 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_work to positive_review

comment:23 Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict, try next beta

comment:24 Changed 3 years ago by git

  • Commit changed from 8322fd5fe18b1bb624b78fbeccf2a8c37bd5e69f to ed5070dd7ed52b0cbb405cb77cb651fb6d6f2f38

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

ed5070dMerging 7.3.beta7

comment:25 Changed 3 years ago by andrew.mathas

  • Status changed from needs_work to positive_review

Merged into 7.3.beta7. Tests pass.

comment:26 Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work

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]

comment:27 follow-up: Changed 3 years ago by 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.

comment:28 Changed 3 years ago by git

  • Commit changed from ed5070dd7ed52b0cbb405cb77cb651fb6d6f2f38 to ba7dd9836386ed119c257d9bb42030149e091b47

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

ba7dd98Fixing hashed doct-test

comment:29 in reply to: ↑ 27 Changed 3 years ago by andrew.mathas

  • Status changed from needs_work to positive_review

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.

comment:30 Changed 3 years ago by vbraun

  • Branch changed from public/misc/pickling_global_options-18555 to ba7dd9836386ed119c257d9bb42030149e091b47
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.