Opened 3 months ago

Closed 6 weeks ago

#30402 closed defect (fixed)

Cannot load an object containing a matrix if it was saved from Python 2 Sage

Reported by: gh-penguian Owned by:
Priority: blocker Milestone: sage-9.2
Component: pickling Keywords: load, save, matrix
Cc: nbruin, jhpalmieri, chapoton, slelievre Merged in:
Authors: Nils Bruin, John Palmieri, Jonathan Kliem Reviewers: Dima Pasechnik, Paul Leopardi
Report Upstream: N/A Work issues:
Branch: c352b8a (Commits) Commit: c352b8ae5c59a3113ef35c888ca6118a322a6f45
Dependencies: Stopgaps:

Description (last modified by mkoeppe)

I am currently trying to convert my Boolean-Cayley-graphs project from Python 2 to Python 3 Sage. https://github.com/penguian/Boolean-Cayley-graphs/tree/23-port-to-python-3

I have succeeded in converting the code, but cannot load objects that were saved by my previous Python 2-based code. These objects contain matrices as members.

As far as I can tell, load(...,encoding='latin-1') fails to load a dict containing a matrix but load(...,encoding='bytes') loads but does not restore the original object. See the attachments, showing a script run on Cocalc, and its results.

Along the way, we fix some code style etc. in `src/sage/matrix/matrix_integer_dense.pyx'.b

Blocker for 9.2 because this defect is in the way of adoption of python3-based Sage versions by users.

Attachments (2)

test-save.sage (1.3 KB) - added by gh-penguian 3 months ago.
Test script to run on Cocalc
matrix-load-save-bug-examples.txt (9.4 KB) - added by gh-penguian 3 months ago.
Results of running test script with different versions of Sage

Download all attachments as: .zip

Change History (23)

Changed 3 months ago by gh-penguian

Test script to run on Cocalc

Changed 3 months ago by gh-penguian

Results of running test script with different versions of Sage

comment:1 Changed 3 months ago by gh-penguian

  • Description modified (diff)

comment:3 Changed 3 months ago by gh-penguian

I applied the change suggested by Nils Bruin to Sage 9.1 installed from source on Kubuntu 20.04, and it works for me.

comment:4 Changed 3 months ago by mkoeppe

  • Cc nbruin jhpalmieri added
  • Priority changed from major to critical

Could someone please prepare a branch for this ticket?

comment:5 Changed 3 months ago by mkoeppe

  • Cc chapoton slelievre added

comment:6 Changed 7 weeks ago by mkoeppe

It would be really nice to get this fix in.

comment:7 Changed 7 weeks ago by jhpalmieri

I have a branch, but I am having problems pushing it to trac. If someone else wants to prepare it, go ahead.

  • src/sage/matrix/matrix_integer_dense.pyx

    diff --git a/src/sage/matrix/matrix_integer_dense.pyx b/src/sage/matrix/matrix_integer_dense.pyx
    index fa6971504b..98aee236eb 100644
    a b cdef class Matrix_integer_dense(Matrix_dense): 
    550550        return data
    551551
    552552    def _unpickle(self, data, int version):
     553        """
     554        TESTS::
     555
     556            sage: b = matrix(ZZ,2,3, [0,0,0, 0, 0, 0])
     557            sage: s = b'1 61 f -2 3 0'
     558            sage: t = s.decode()
     559            sage: b._unpickle(s, 0) == b._unpickle(t, 0)
     560            True
     561            sage: b
     562            [  1 193  15]
     563            [ -2   3   0]
     564        """
    553565        if version == 0:
     566            if isinstance(data, str):
     567                # old Py2 pickle: old "bytes" object reaches us as a
     568                # latin1-encoded string.
     569                data = data.encode('latin1')
    554570            if isinstance(data, bytes):
    555571                self._unpickle_version0(data)
    556572            elif isinstance(data, list):

Needs testing.

comment:8 Changed 7 weeks ago by gh-kliem

I prepared it and I have the same problem, it seems:

kliem@cofio:~/localhome/sage/src/sage/matrix$ git push --set-upstream trac public/30402

perl: warning: Setting locale failed.
perl: warning: Please check that your locale settings:
	LANGUAGE = (unset),
	LC_ALL = (unset),
	LC_PAPER = "de_DE.UTF-8",
	LC_ADDRESS = "de_DE.UTF-8",
	LC_MONETARY = "de_DE.UTF-8",
	LC_NUMERIC = "de_DE.UTF-8",
	LC_TELEPHONE = "de_DE.UTF-8",
	LC_IDENTIFICATION = "de_DE.UTF-8",
	LC_MEASUREMENT = "de_DE.UTF-8",
	LC_TIME = "de_DE.UTF-8",
	LC_NAME = "de_DE.UTF-8",
	LANG = "en_US.UTF-8"
    are supported and installed on your system.
perl: warning: Falling back to the standard locale ("C").
Enumerating objects: 35, done.
Counting objects: 100% (35/35), done.
Delta compression using up to 8 threads
Compressing objects: 100% (30/30), done.
Writing objects: 100% (30/30), 3.57 KiB | 3.57 MiB/s, done.
Total 30 (delta 25), reused 0 (delta 0)
remote: error: insufficient permission for adding an object to repository database ./objects
remote: fatal: failed to write object
error: remote unpack failed: unpack-objects abnormal exit
To trac.sagemath.org:sage.git
 ! [remote rejected]       public/30402 -> public/30402 (unpacker error)
error: failed to push some refs to 'git@trac.sagemath.org:sage.git'

comment:9 Changed 7 weeks ago by dimpase

please try again now. I've been doing some maintainance/repairing of the git repo.

comment:10 Changed 7 weeks ago by gh-kliem

same error

comment:11 Changed 7 weeks ago by dimpase

OK, let me see.

comment:12 Changed 7 weeks ago by dimpase

  • Branch set to public/ticket/30402-py23
  • Commit set to 342030c403a30a41b87c7efbf977d79bf1ca4afb
  • Status changed from new to needs_review

I've fixed the permissions on ...repository database ./objects and was able to push the branch.

There seems to be some problem there caused by gitolite - the thing that enables viewing branches here in the browser, which also uses the repo on the server. Weird.


New commits:

342030cfix (un)pickle

comment:13 Changed 7 weeks ago by dimpase

  • Authors set to Nils Bruin, John Palmieri
  • Reviewers set to Dima Pasechnik, Paul Leopardi
  • Status changed from needs_review to positive_review

ok, the test passes. Good to go.

comment:14 Changed 7 weeks ago by gh-kliem

I fixed some codestyle and stuff. Should I push it or put it on a new ticket.

comment:15 Changed 7 weeks ago by dimpase

push it here if you like, why not.

comment:16 Changed 7 weeks ago by git

  • Commit changed from 342030c403a30a41b87c7efbf977d79bf1ca4afb to c352b8ae5c59a3113ef35c888ca6118a322a6f45
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

d798a42fix codestyle etc.
44e5151pycodestyle
c352b8aadded indirect doctest flag

comment:17 Changed 7 weeks ago by gh-kliem

Ok.

Maybe one day sage -tox for all of sage :-)

It doesn't even pass this file yet due to coverage and it not liking the variable name ans.

comment:18 Changed 7 weeks ago by dimpase

  • Status changed from needs_review to positive_review

OK, great, thanks. Please add yourself to reviewers/authors, if you like.

comment:19 Changed 7 weeks ago by gh-kliem

  • Authors changed from Nils Bruin, John Palmieri to Nils Bruin, John Palmieri, Jonathan Kliem
  • Description modified (diff)

comment:20 Changed 7 weeks ago by mkoeppe

  • Description modified (diff)
  • Priority changed from critical to blocker

comment:21 Changed 6 weeks ago by vbraun

  • Branch changed from public/ticket/30402-py23 to c352b8ae5c59a3113ef35c888ca6118a322a6f45
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.