Remove some py2 tags in explain_pickle

[fchapoton]

We remove some py2 tags from explain_pickle doctests which we still get correct results (with python3).

CC: @embray @mjungmath @tscrim

Component: python3

Author: Frédéric Chapoton

Branch/Commit: 131841f

Reviewer: Travis Scrimshaw, Kwankyu Lee

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

[fchapoton]

Branch: u/chapoton/33144

[fchapoton]

Commit: 9d0b7a1

[fchapoton]

comment:1

boring and painful update..

any expert on pickles aboard ?

---

New commits:

9d0b7a1

removal of py2 tag in doctests of explain pickle (first wave)

[fchapoton]

comment:3

ok, let's do that in several steps. Please review this first step.

[fchapoton]

comment:6

please somebody have a look

This is the last file containing #py2 tags. Here I remove only the simple ones, and the changes should rather be uncontroversial. The remaining cases are out of my capacity, and require an expert of pickle, that we may no longer have anywhere in sight..

One alternative would be #27350 (externalize) or just drop the file completely.

[kwankyu]

comment:7

It would be wonderful for Erik Bray to work on #27350 and close this ticket as invalid/wontfix.

If that would not happen, then I think we should fix explain_pickle for python3 before we remove the py2 tag, instead of "fixing" doctests for the broken explain_pickle.

[fchapoton]

comment:8

As far as I understand, Erik is no longer available.

[tscrim]

Reviewer: Travis Scrimshaw

[tscrim]

comment:9

In principle, I agree with #27350, but I am slightly worried about the practice of it since I find this tool to be very useful at times and would be saddened if it bitrotted. I, unfortunately, would not be able to maintain such a package as I don't understand how explain_pickle() works. Since these doctests seem to just be updating the output, I think we should merge these in now. Likewise, I don't think I could help with any of the further marked tests.

Kwankyu, any objections to me setting a positive review?

[kwankyu]

comment:10

Replying to @tscrim:

Since these doctests seem to just be updating the output,

Some outputs from explain_pickle (via test_pickle), for instance,

             sage: from sage.misc.explain_pickle import *
-            sage: test_pickle(dumps(100000r, compress=False))  # py2
-                0: \x80 PROTO      2
-                2: J    BININT     100000
-                7: .    STOP
-            highest protocol among opcodes = 2
-            explain_pickle in_current_sage=True/False:
-            100000
-            result: 100000
+            sage: test_pickle(dumps(100000r, compress=False))
+                    0: \x80 PROTO      2
+                    2: c    GLOBAL     '_codecs encode'
+                   18: q    BINPUT     0
+                   20: X    BINUNICODE '\x80\x02J\xa0\x86\x01\x00.'
+                   36: q    BINPUT     1
+                   38: X    BINUNICODE 'latin1'
+                   49: q    BINPUT     2
+                   51: \x86 TUPLE2
+                   52: q    BINPUT     3
+                   54: R    REDUCE
+                   55: q    BINPUT     4
+                   57: .    STOP
+                highest protocol among opcodes = 2
+                explain_pickle in_current_sage=True:
+                from _codecs import encode
+                encode('\x80\x02J\xa0\x86\x01\x00.', 'latin1')
+                explain_pickle in_current_sage=False:
+                pg_encode = unpickle_global('_codecs', 'encode')
+                pg_encode('\x80\x02J\xa0\x86\x01\x00.', 'latin1')
+                result: b'\x80\x02J\xa0\x86\x01\x00.'

seem wrong, which indicates it worked fine with py2 but is now broken with py3. Do we ever correct doctests for wrong output? I think leaving them with py2 tag is right if we do not fix the broken explain_pickle.

Indentations of outputs are wrong.

[tscrim]

comment:11

Replying to @kwankyu:

Replying to @tscrim:

Since these doctests seem to just be updating the output,

seem wrong, which indicates it worked fine with py2 but is now broken with py3. Do we ever correct doctests for wrong output? I think leaving them with py2 tag is right if we do not fix the broken explain_pickle.

The pickle itself has changed. If you run loads(dumps(100000r)), it returns the correct object without error. In this case, it is likely because int has changed from Python2 to Python3.

Indentations of outputs are wrong.

Indeed; although I found the extra indentation level helped make clear what the doctest outputs were since they are so long with their own indentation. I don't hold a strong opinion on changing this or not.

[kwankyu]

comment:12

Replying to @tscrim:

Replying to @kwankyu:

Replying to @tscrim:

Since these doctests seem to just be updating the output,

seem wrong, which indicates it worked fine with py2 but is now broken with py3. Do we ever correct doctests for wrong output? I think leaving them with py2 tag is right if we do not fix the broken explain_pickle.

The pickle itself has changed.

I meant the result. Shouldn't it be 100000?

[tscrim]

comment:13

Replying to @kwankyu:

Replying to @tscrim:

Replying to @kwankyu:

Replying to @tscrim:

Since these doctests seem to just be updating the output,

seem wrong, which indicates it worked fine with py2 but is now broken with py3. Do we ever correct doctests for wrong output? I think leaving them with py2 tag is right if we do not fix the broken explain_pickle.

The pickle itself has changed.

I meant the result. Shouldn't it be 100000?

I believe it is the right result but represented or interpreted in the wrong format. Mainly the bytes returned are not converted to a string correctly or something similar.

[tscrim]

comment:14

Perhaps we just don't remove the tag in the cases when the result is (at least superficially) different?

[kwankyu]

comment:15

Replying to @tscrim:

Perhaps we just don't remove the tag in the cases when the result is (at least superficially) different?

+1

[fchapoton]

Changed branch from u/chapoton/33144 to public/ticket/33144-short

[fchapoton]

comment:16

here is a new branch, where I have dropped the changes that did no look right

I may have missed some

---

New commits:

cf9a6d9

remove only some simple #py2 tags in explain_pickle

[fchapoton]

Changed commit from 9d0b7a1 to cf9a6d9

[kwankyu]

comment:17

How about indentations? Did you indent more spaces intentionally?

[kwankyu]

comment:18

I think outputs should look as they do on the command line, modulo wrappings.

[sagetrac-git]

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

3c82b13

remove some #py2 tags in explain_pickle

[sagetrac-git]

Changed commit from cf9a6d9 to 3c82b13

[fchapoton]

comment:20

now the same as previous one, only with less indentations

[kwankyu]

comment:21

Thanks. But some cases with different result are still in.

[fchapoton]

patch for removing some #py2 tags

[fchapoton]

comment:22

Attachment: 0001-removal-of-py2-tag-in-doctests-of-explain-pickle-fir.patch.gz

Then maybe the simplest way would be if you could please edit the attached patch file : keep only the changes that you find appropriate and make a branch from that modified patch.

[sagetrac-git]

Changed commit from 3c82b13 to 131841f

[sagetrac-git]

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

b47176e	Merge branch 'develop'
131841f	Restore py2 tag for cases with different result

[kwankyu]

comment:24

Replying to @fchapoton:

Then maybe the simplest way would be if you could please edit the attached patch file : keep only the changes that you find appropriate and make a branch from that modified patch.

That is done over your last commit.

[kwankyu]

Changed reviewer from Travis Scrimshaw to Travis Scrimshaw, Kwankyu Lee

[kwankyu]

comment:25

I am positive now.

[fchapoton]

comment:26

thanks, you can set to positive if you wish

[kwankyu]

comment:27

Thanks.

[vbraun]

Changed branch from public/ticket/33144-short to 131841f