Opened 3 years ago

Closed 2 years ago

#24786 closed defect (fixed)

py3: various Python 3 bugs in the brial package

Reported by: embray Owned by:
Priority: major Milestone: sage-8.4
Component: python3 Keywords: brial
Cc: jdemeyer Merged in:
Authors: Frédéric Chapoton Reviewers: Erik Bray
Report Upstream: Fixed upstream, in a later stable release. Work issues:
Branch: 22fc642 (Commits) Commit: 22fc642cf90d084584361bef53828993f4e72140
Dependencies: Stopgaps:

Description (last modified by chapoton)

This error occurs in several places; e.g.

sage -t src/sage/rings/polynomial/multi_polynomial_sequence.py
**********************************************************************
File "src/sage/rings/polynomial/multi_polynomial_sequence.py", line 106, in sage.rings.polynomial.multi_polynomial_sequence
Failed example:
    C[0].groebner_basis()
Exception raised:
    Traceback (most recent call last):
      File "/home/embray/src/sagemath/sage-python3/local/lib/python3.6/site-packages/sage/doctest/forker.py", line 556, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/embray/src/sagemath/sage-python3/local/lib/python3.6/site-packages/sage/doctest/forker.py", line 966, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.rings.polynomial.multi_polynomial_sequence[7]>", line 1, in <module>
        C[Integer(0)].groebner_basis()
      File "/home/embray/src/sagemath/sage-python3/local/lib/python3.6/site-packages/sage/rings/polynomial/multi_polynomial_sequence.py", line 530, in groebner_basis
        return self.ideal().groebner_basis(*args, **kwargs)
      File "sage/rings/polynomial/pbori.pyx", line 5070, in sage.rings.polynomial.pbori.BooleanPolynomialIdeal.groebner_basis (build/cythonized/sage/rings/polynomial/pbori.cpp:42826)
        gb = self._groebner_basis(**kwds)
      File "sage/rings/polynomial/pbori.pyx", line 5100, in sage.rings.polynomial.pbori.BooleanPolynomialIdeal._groebner_basis (build/cythonized/sage/rings/polynomial/pbori.cpp:43222)
        from brial.gbcore import groebner_basis
      File "/home/embray/src/sagemath/sage-python3/local/lib/python3.6/site-packages/brial/__init__.py", line 31, in <module>
        from .PyPolyBoRi import *
      File "/home/embray/src/sagemath/sage-python3/local/lib/python3.6/site-packages/brial/PyPolyBoRi.py", line 74, in <module>
        OrderCode.__dict__ = order_dict
    AttributeError: attribute '__dict__' of 'type' objects is not writable

There are also several other minor Python 3 bugs in brial.

Upstream PR: https://github.com/BRiAl/BRiAl/pull/33

New release of BRial: https://github.com/BRiAl/BRiAl/releases/download/1.2.4/brial-1.2.4.tar.bz2

Change History (32)

comment:1 Changed 3 years ago by chapoton

Erik, could you please post on sage-devel some kind of report of your progress on python3 ? In particular, I would like to know how far we still are

(0) from sage starting (almost there, I think it may just need #24728),

(1) from being able to build the doc (#24774 and then ?),

(2) from having a functional doctest framework (#24343 and what else ?).

And what are the involved tickets for each of these objectives ? I must say that I am a bit lost in your recent flurry of activity. I would like to put some priority between all these tickets.

comment:2 Changed 3 years ago by embray

That's fair--I've been thinking about that myself. I almost feel like we need some kind of status board for Python 3. I'll see what I can come up with.

As for the "recent flurry of activity" almost all of this is work I did some time ago but never got around to organizing into comprehensible discrete tickets. Most of it is miscellaneous and not directly tied to each other (I will usually make clear when multiple tickets are related to each other).

Last edited 3 years ago by embray (previous) (diff)

comment:3 Changed 3 years ago by chapoton

Here are the lines in https://github.com/BRiAl/BRiAl/blob/master/sage-brial/brial/PyPolyBoRi.py

class OrderCode:
    pass

OrderCode.__dict__ = order_dict
OrderCode.__module__ = __name__

comment:4 Changed 3 years ago by fbissey

There is a newer version of brial but if there is still bugs I am happy to merge PR.

comment:5 Changed 3 years ago by embray

  • Description modified (diff)
  • Milestone changed from sage-8.2 to sage-8.3
  • Report Upstream changed from Not yet reported upstream; Will do shortly. to Reported upstream. No feedback yet.
  • Status changed from new to needs_review
  • Summary changed from py3: error in brial.PyPolyBoRi to py3: various Python 3 bugs in the brial package

comment:6 Changed 2 years ago by saraedum

  • Status changed from needs_review to needs_info

This says "needs review". What should be reviewed here?

comment:7 Changed 2 years ago by embray

  • Report Upstream changed from Reported upstream. No feedback yet. to Fixed upstream, but not in a stable release.

The PR to brial did need review, but this is done now.

What we need to do now is either add this patch to Sage, or release a new version of brial and update Sage to use it (preferably the latter). But let's wait to see if I can identify any more Python 3 bugs in brial (I don't think there are but it's feasible).

comment:8 Changed 2 years ago by fbissey

I can release a new brial when you think it is ready. The main reason I have that power on the brial repo is to be able to do releases for sage in a timely fashion. Although I also did quite a bit of clean up with the help of other sage-on-distro maintainers in the last few months.

comment:9 follow-up: Changed 2 years ago by chapoton

@fbissey

Please make a release of brial. This will be helpful for python3 progress.

comment:10 in reply to: ↑ 9 Changed 2 years ago by fbissey

Replying to chapoton:

@fbissey

Please make a release of brial. This will be helpful for python3 progress.

I may have time tomorrow but that will be a squeeze. Better count on it on Tuesday (NZ time).

comment:11 Changed 2 years ago by fbissey

New release out for you at https://github.com/BRiAl/BRiAl/releases/download/1.2.4/brial-1.2.4.tar.bz2

Do check that it has all the changes you need.

comment:12 Changed 2 years ago by chapoton

  • Report Upstream changed from Fixed upstream, but not in a stable release. to Fixed upstream, in a later stable release.
  • Status changed from needs_info to needs_work

now we need to update brial here

comment:13 Changed 2 years ago by embray

Are you saying I should make a branch to upgrade Sage's brial to this version?

comment:14 Changed 2 years ago by chapoton

Hello Erik, and welcome back.

Well, I think we should upgrade brial, indeed. Either here or in another ticket.

Maybe Pynac upgrade is more important than this one for being able to make the doc.

comment:15 Changed 2 years ago by embray

Thanks!

And yes, IIRC the pynac updates are especially critical for Python 3 support (otherwise there are segfaults, etc.)

comment:16 Changed 2 years ago by embray

I can put the brial update in this ticket.

comment:17 Changed 2 years ago by chapoton

@embray : yes, please, let us do the brial update here

comment:18 Changed 2 years ago by chapoton

  • Description modified (diff)

comment:19 Changed 2 years ago by chapoton

  • Branch set to public/brial_1.2.4
  • Commit set to fcba0bf5ba89b70c35553c215493ac4d1a94964b

New commits:

fcba0bfupdate brial to 1.2.4

comment:20 Changed 2 years ago by chapoton

patches do not apply.. And I am not able to do the required refreshing of patches, sorry

comment:21 Changed 2 years ago by git

  • Commit changed from fcba0bf5ba89b70c35553c215493ac4d1a94964b to 22fc642cf90d084584361bef53828993f4e72140

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

22fc642removing brial patch

comment:22 Changed 2 years ago by chapoton

  • Cc jdemeyer added
  • Status changed from needs_work to needs_review

turns out the patch was no longer needed. So this should be good now

comment:23 Changed 2 years ago by chapoton

  • Status changed from needs_review to needs_work

sage crash..

comment:24 follow-up: Changed 2 years ago by chapoton

  • Status changed from needs_work to needs_review

well, seems to work after sage -f brial

so setting back to needs review.

comment:25 in reply to: ↑ 24 Changed 2 years ago by fbissey

Replying to chapoton:

well, seems to work after sage -f brial

so setting back to needs review.

I was a bit surprised and was going to ask for more details.

comment:26 follow-up: Changed 2 years ago by embray

LGTM. Jeroen should probably comment since he wrote the patch.

comment:27 in reply to: ↑ 26 Changed 2 years ago by fbissey

Replying to embray:

LGTM. Jeroen should probably comment since he wrote the patch.

Why? The patch was from a pull request he made to BRiAl and which I have merged. So the patch is included in this version of BRiAl. https://github.com/BRiAl/BRiAl/pull/15

comment:28 Changed 2 years ago by embray

  • Reviewers set to Erik Bray
  • Status changed from needs_review to positive_review

Ok. That wasn't immediately obvious. It looks like my PR is included as well.

comment:29 Changed 2 years ago by tscrim

Author name.

comment:30 Changed 2 years ago by chapoton

  • Authors set to Frédéric Chapoton

comment:31 Changed 2 years ago by embray

  • Milestone changed from sage-8.3 to sage-8.4

I believe this issue can reasonably be addressed for Sage 8.4.

comment:32 Changed 2 years ago by vbraun

  • Branch changed from public/brial_1.2.4 to 22fc642cf90d084584361bef53828993f4e72140
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.