Opened 4 months ago

Closed 3 months ago

Last modified 3 months ago

#27065 closed task (fixed)

py3: handle the explain_pickle problem

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.7
Component: python3 Keywords:
Cc: embray, jdemeyer, fbissey, tscrim, vklein Merged in:
Authors: Frédéric Chapoton Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: aa3173c (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by slelievre)

The file explain_pickle has currently 70 failing doctests with python3. It seems to be very difficult to fix them all.

Proposal:

  • either (1) remove this file completely
  • or (2) tag all doctests with #py2

This ticket implements option (2): tag all doctests with #py2.

Further work on explain_pickle is tracked at #27350.

Change History (28)

comment:1 Changed 4 months ago by chapoton

  • Description modified (diff)
  • Status changed from new to needs_info

comment:2 Changed 4 months ago by embray

I brought this up on the mailing list some months ago, and consensus of most people who participated in the discussion was to remove this module from Sage, but publish it as a stand-alone package: https://groups.google.com/d/msg/sage-devel/94kP0_Xbx04/x7St89zwCgAJ

I can work on that. My feeling about it is to make two completely separate Python 2 and Python 3 versions. Maintenance-wise I don't see that being much of a burden since this module won't likely be maintained much anyways, and after 2020 the Python 2 version can be removed entirely.

comment:3 Changed 4 months ago by chapoton

Good. Should we deprecate or can we just remove (given our python3 objective) ?

comment:4 Changed 4 months ago by chapoton

  • Branch set to public/ticket/27065
  • Commit set to 18bfcb98444219141f0fe2296059faa5cebffeef

Here is branch that removes explain_pickle from sage.


New commits:

18bfcb9full removal of explain_pickle (turned into an external python package)
Last edited 4 months ago by chapoton (previous) (diff)

comment:5 Changed 4 months ago by chapoton

  • Milestone changed from sage-8.6 to sage-8.7

comment:6 Changed 4 months ago by git

  • Commit changed from 18bfcb98444219141f0fe2296059faa5cebffeef to 25afb287e294d59341ff4647f128178be227d83b

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

ead257cMerge branch 'public/ticket/27065' in 8.6
25afb28trac 27065 fixing some doctests

comment:7 Changed 4 months ago by chapoton

What to do with the few doctests using unpickle_newobj and unpickle_build ? Just remove them ?

comment:8 Changed 4 months ago by embray

  • Status changed from needs_info to needs_work

I had assumed keep sage.misc.explain_pickle but just have it do from explain_pickle import * and also raise a deprecation warning if imported.

This will have to wait, of course, until I make explain_pickle, and we can add it either as a standard package, or an optional package (in which case sage.misc.explain_pickle should catch ImportError and provide some explanation).

comment:9 Changed 4 months ago by tscrim

I would recommend making it straight to standard, but this will need a (quick) sage-devel vote.

comment:10 Changed 4 months ago by embray

I'm fine with either way, but point is there should still be some sort of regression for the old module (even if probably very few people use it outside of development--but clearly some do!)

comment:11 Changed 4 months ago by git

  • Commit changed from 25afb287e294d59341ff4647f128178be227d83b to cd63b0c05631198a35d8680cde0e18e639dd6692

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

cd63b0ctrac 27065 mark the doctests as "optional - explain_pickle"

comment:12 Changed 4 months ago by chapoton

  • Status changed from needs_work to needs_review

Now ready. All failing doctests have been marked as depending on the non-existing pip package "explain_pickle".

comment:13 Changed 4 months ago by embray

  • Status changed from needs_review to needs_work

Please just let me take care of it. I promise I will soon (although I don't consider it high priority even for the Python 3 port, but I know you want to get those failures down--though this would be a good use case for known-failures :)

I fear you did some work here that was ultimately unnecessary. In particular, I believe the unpickle_newobj utility doesn't even belong in explain_pickle in the first place; it should be moved elsewhere.

Second of all, I had still planned to keep the sage.misc.explain_pickle module as a wrapper--possibly with parts of it deprecated, but also possibly not; it depends on exactly how the third-party explain_pickle module ends up being structured. For example, there's lots of use currently of Sage-specific stuff like SageInputExpression and SageInputBuilder that doesn't necessarily make sense to use (at least directly) in a more generic implementation. But it will definitely have an API that will allow using them, but that might involve extensions that would still belong somewhere in sage, such as in the existing explain_pickle module.

So it's not just a simple matter of removing that module and declaring tests that happened to use it as optional, and there's not much point in doing work on this until I have that third-party module ready to work with.

Last edited 4 months ago by embray (previous) (diff)

comment:14 Changed 4 months ago by embray

In fact, now that I've done a bit more research into how this works, I'm almost certain that sage.misc.explain_pickle won't go away at all. However, most of its current contents will be gone, and replaced with a few Sage-specific wrappers and some tests for cases that are of particular interest for Sage (e.g. testing that register_unpickle_override is handled properly when explaining pickles that load globals).

comment:15 Changed 4 months ago by embray

  • Owner changed from (none) to embray

I've spent some time better understanding exactly how explain_pickle works, and believe I have a strategy now for implementing the "generic" version, and possibly deprecating parts of the existing code.

comment:16 Changed 4 months ago by embray

Making progress on this, though I still need to do a lot of testing, and the integration back into Sage isn't even started yet.

comment:17 Changed 3 months ago by chapoton

Erik, any news on this front ? I am tempted to just put # py2 everywhere in this file..

comment:19 Changed 3 months ago by chapoton

  • Branch changed from public/ticket/27065 to u/chapoton/py2_tag_explain_pickle
  • Cc vklein added
  • Commit changed from cd63b0c05631198a35d8680cde0e18e639dd6692 to aa3173c15e55cd6f372382888a956d34178f4497
  • Status changed from needs_work to needs_review

Thanks, Erik. I understand very well the existence of "other priorities".

I am proposing now a simple branch, where I just tag #py2 the failing doctests. This will allow to put this aside, and it will no longer stand in the middle of the way to python3-full-tests-pass.


New commits:

aa3173cpy3: decorate most doctests in explain_pickle with tag py2

comment:20 Changed 3 months ago by chapoton

green bot, please review

comment:21 Changed 3 months ago by chapoton

  • Authors set to Frédéric Chapoton

comment:22 Changed 3 months ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

I agree, this is better than nothing.

comment:23 Changed 3 months ago by embray

  • Status changed from positive_review to needs_work

I don't really see the urgency of this. It's really easy to ignore these failures as they're completely localized to this module, which can easily be skipped entirely.

Feel free to set back to positive_review if you disagree, but please open a new ticket for the continued work and assign me or else I will probably forget to work on it at all (I want to though; last I worked on it it was about 80% complete--it was just surprisingly hard to get some things working on Python 3).

comment:24 Changed 3 months ago by chapoton

  • Status changed from needs_work to positive_review

I have created #27350 for the task you want to do.

I am now setting back the present trivial ticket to positive.

comment:25 Changed 3 months ago by embray

  • Owner changed from embray to (none)

comment:26 Changed 3 months ago by embray

Okay, thank you. This is fine with me if it will make you happy. However, though it's not a big deal, in the future I would prefer that on tickets where the "owner" field is set to me where I am in the process of working on a solution, that the ticket not be essentially hijacked, and instead that a new ticket be created for your temporary workaround.

comment:27 Changed 3 months ago by vbraun

  • Branch changed from u/chapoton/py2_tag_explain_pickle to aa3173c15e55cd6f372382888a956d34178f4497
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:28 Changed 3 months ago by slelievre

  • Commit aa3173c15e55cd6f372382888a956d34178f4497 deleted
  • Description modified (diff)
Note: See TracTickets for help on using tickets.