py3: handle the explain_pickle problem

[fchapoton]

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.

CC: @embray @jdemeyer @kiwifb @tscrim @vinklein

Component: python3

Author: Frédéric Chapoton

Branch: aa3173c

Reviewer: Jeroen Demeyer

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

[embray]

comment:2

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.

[fchapoton]

comment:3

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

[fchapoton]

comment:4

Here is branch that removes explain_pickle from sage.

---

New commits:

18bfcb9

full removal of explain_pickle (turned into an external python package)

[fchapoton]

Commit: 18bfcb9

[fchapoton]

Branch: public/ticket/27065

[sagetrac-git]

Changed commit from 18bfcb9 to 25afb28

[sagetrac-git]

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

ead257c	Merge branch 'public/ticket/27065' in 8.6
25afb28	trac 27065 fixing some doctests

[fchapoton]

comment:7

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

[embray]

comment:8

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).

[tscrim]

comment:9

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

[embray]

comment:10

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!)

[sagetrac-git]

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

cd63b0c

trac 27065 mark the doctests as "optional - explain_pickle"

[sagetrac-git]

Changed commit from 25afb28 to cd63b0c

[fchapoton]

comment:12

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

[embray]

comment:13

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.

[embray]

comment:14

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).

[embray]

comment:15

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.

[embray]

comment:16

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.

[fchapoton]

comment:17

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

[embray]

comment:18

I have had other priorities come up since I last worked on this (see e.g. https://trac.sagemath.org/query?priority=blocker&status=needs_info&status=needs_review&status=needs_work&status=new&status=positive_review&milestone=sage-8.7&col=id&col=summary&col=priority&col=status&col=type&col=milestone&col=component&order=priority)

If I have updates I won't just sit silently on them.

[fchapoton]

Changed commit from cd63b0c to aa3173c

[fchapoton]

comment:19

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:

aa3173c

py3: decorate most doctests in explain_pickle with tag py2

[fchapoton]

Changed branch from public/ticket/27065 to u/chapoton/py2_tag_explain_pickle

[fchapoton]

comment:20

green bot, please review

[fchapoton]

Author: Frédéric Chapoton

[jdemeyer]

comment:22

I agree, this is better than nothing.

[jdemeyer]

Reviewer: Jeroen Demeyer

[embray]

comment:23

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).

[fchapoton]

comment:24

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

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

[embray]

comment:26

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.

[vbraun]

Changed branch from u/chapoton/py2_tag_explain_pickle to aa3173c

[slel]

Changed commit from aa3173c to none