Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update pari to 2.15.4, drop patch #35302

Merged
merged 9 commits into from
Oct 31, 2023
Merged

update pari to 2.15.4, drop patch #35302

merged 9 commits into from
Oct 31, 2023

Conversation

dimpase
Copy link
Member

@dimpase dimpase commented Mar 17, 2023

📚 Description

update pari to 2.15.4, drop patch

Will fix #35219

📝 Checklist

  • I have made sure that the title is self-explanatory and the description concisely explains the PR.

@orlitzky
Copy link
Contributor

I updated it in ::gentoo. The changelog looks safe but I haven't run the sage test suite yet. The "Build & Test" CI has a green checkmark but looks like it crashed to me?

@dimpase
Copy link
Member Author

dimpase commented Mar 18, 2023

What has crashed?
https://github.com/sagemath/sage/actions/runs/4446987505/jobs/7807969635?pr=35302 says "all tests passed"

@orlitzky
Copy link
Contributor

It also won't let me upload a screenshot. The raw logs look OK, it's only the summary that's busted.

@yyyyx4
Copy link
Member

yyyyx4 commented Mar 19, 2023

On the topic of patching PARI, please have a look at #35219.

@codecov-commenter
Copy link

Codecov Report

Patch coverage has no change and project coverage change: -0.02 ⚠️

Comparison is base (c00e6c2) 88.62% compared to head (c688bfd) 88.60%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #35302      +/-   ##
===========================================
- Coverage    88.62%   88.60%   -0.02%     
===========================================
  Files         2148     2148              
  Lines       398855   398855              
===========================================
- Hits        353480   353409      -71     
- Misses       45375    45446      +71     
Impacted Files Coverage Δ
src/sage/arith/misc.py 90.99% <ø> (ø)

... and 28 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

src/sage/arith/misc.py Outdated Show resolved Hide resolved
@vbraun
Copy link
Member

vbraun commented Mar 27, 2023

File "src/sage/interfaces/gp.py", line 619, in sage.interfaces.gp.Gp._next_var_name
Failed example:
    for n in [1..13000]:  # long time
        a = g(n)          # long time
Exception raised:
    Traceback (most recent call last):
      File "/home/release/Sage/src/sage/interfaces/expect.py", line 523, in _start
        self._expect.expect(self._prompt)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.11.1/lib/python3.11/site-packages/pexpect/spawnbase.py", line 343, in expect
        return self.expect_list(compiled_pattern_list,
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/home/release/Sage/local/var/lib/sage/venv-python3.11.1/lib/python3.11/site-packages/pexpect/spawnbase.py", line 372, in expect_list
        return exp.expect_loop(timeout)
               ^^^^^^^^^^^^^^^^^^^^^^^^
      File "/home/release/Sage/local/var/lib/sage/venv-python3.11.1/lib/python3.11/site-packages/pexpect/expect.py", line 179, in expect_loop
        return self.eof(e)
               ^^^^^^^^^^^
      File "/home/release/Sage/local/var/lib/sage/venv-python3.11.1/lib/python3.11/site-packages/pexpect/expect.py", line 122, in eof
        raise exc
    pexpect.exceptions.EOF: End Of File (EOF). Exception style platform.
    PARI/GP interpreter finished running /home/release/Sage/local/bin/gp --fast --emacs --quiet --stacksize 10000
    command: /home/release/Sage/local/bin/gp
    args: ['/home/release/Sage/local/bin/gp', '--fast', '--emacs', '--quiet', '--stacksize', '10000']
    buffer (last 100 chars): b''
    before (last 100 chars): b" [hint] set 'parisizemax' to a nonzero value in your GPRC\r\n\r\n### Errors on startup, exiting...\r\n\r\n\r\n"
    after: <class 'pexpect.exceptions.EOF'>
    match: None
    match_index: None
    exitstatus: 1
    flag_eof: True
    pid: 2983226
    child_fd: 23
    closed: False
    timeout: None
    delimiter: <class 'pexpect.exceptions.EOF'>
    logfile: None
    logfile_read: None
    logfile_send: None
    maxread: 4194304
    ignorecase: False
    searchwindowsize: None
    delaybeforesend: None
    delayafterclose: 0.1
    delayafterterminate: 0.1
    searcher: searcher_re:
        0: re.compile(b'\\? ')

And indeed doesn't start with stacksize 10kb:

[release@zen Sage]$ /home/release/Sage/local/bin/gp --fast --emacs --quiet --stacksize 10000
  ***   the PARI stack overflows !
  current stack size: 10000 (0.010 Mbytes)
  [hint] set 'parisizemax' to a nonzero value in your GPRC

@dimpase
Copy link
Member Author

dimpase commented Mar 28, 2023

gp --fast --emacs --quiet --stacksize 10000

works for me, e.g. on macOS:

% gp --fast --emacs --stacksize 10000 
                  GP/PARI CALCULATOR Version 2.15.3 (released)
          i386 running darwin (x86-64/GMP-6.2.1 kernel) 64-bit version
    compiled: Mar 14 2023, Apple clang version 14.0.0 (clang-1400.0.29.202)
                           threading engine: pthread
                 (readline v8.2 enabled, extended help enabled)

                     Copyright (C) 2000-2022 The PARI Group

PARI/GP is free software, covered by the GNU General Public License, and comes 
WITHOUT ANY WARRANTY WHATSOEVER.

Type ? for help, \q to quit.
Type ?18 for how to get moral (and possibly technical) support.

parisize = 10000, primelimit = 500000, nbthreads = 8
? 2+2
%1 = 4
? 

and on GitHub's CI.
@vbraun - what platform is that zen ?

@dimpase
Copy link
Member Author

dimpase commented Mar 28, 2023

OK, reproducible on Fedora, --stacksize 3000000 works.

@yyyyx4
Copy link
Member

yyyyx4 commented Apr 4, 2023

Upstream patch is now available at #2469. (Is there any way for me to push to this branch?)

@dimpase
Copy link
Member Author

dimpase commented Apr 4, 2023

everyone can open a PR against the branch of this PR, irrespective of any team membership. I.e. go to my fork and open a PR there, against the correct base.

@dimpase dimpase changed the title update pari to 2.15.3, drop patch update pari to 2.15.4, drop patch Oct 10, 2023
@dimpase
Copy link
Member Author

dimpase commented Oct 10, 2023

I've left tests in (2 new tests in sagelib, and 2 tests in spkg-configure.m4)
These are quick and useful - Pari is unfortunately known for sometimes breaking their own bug fixes...

@dimpase
Copy link
Member Author

dimpase commented Oct 10, 2023

Please review

@mkoeppe mkoeppe added this to the sage-10.2 milestone Oct 24, 2023
@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 25, 2023

vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 26, 2023
    
### 📚 Description

update pari to 2.15.4, drop patch

Will fix sagemath#35219

### 📝 Checklist

- [x] I have made sure that the title is self-explanatory and the
description concisely explains the PR.
    
URL: sagemath#35302
Reported by: Dima Pasechnik
Reviewer(s): Dima Pasechnik, Lorenz Panny, Michael Orlitzky
@vbraun
Copy link
Member

vbraun commented Oct 27, 2023

I'm getting a lot of

**********************************************************************
File "src/sage/rings/integer.pyx", line 1471, in sage.rings.integer.Integer.digits
Failed example:
    n.digits(base=10)[-1]  # slightly slower than str
Exception raised:
    Traceback (most recent call last):
      File "sage/structure/parent.pyx", line 895, in sage.structure.parent.Parent.__call__
        mor = <map.Map> self._convert_from_hash.get(R)
      File "sage/structure/coerce_dict.pyx", line 650, in sage.structure.coerce_dict.MonoDict.get
        raise KeyError(k)
    KeyError: Integer Ring
    During handling of the above exception, another exception occurred:
    Traceback (most recent call last):
      File "sage/structure/parent.pyx", line 2503, in sage.structure.parent.Parent._internal_convert_map_from
        return self._convert_from_hash.get(S)
      File "sage/structure/coerce_dict.pyx", line 650, in sage.structure.coerce_dict.MonoDict.get
        raise KeyError(k)
    KeyError: Integer Ring
    During handling of the above exception, another exception occurred:
    Traceback (most recent call last):
      File "/var/lib/buildbot/slave/sage_git/build/src/sage/doctest/forker.py", line 709, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/var/lib/buildbot/slave/sage_git/build/src/sage/doctest/forker.py", line 1144, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.rings.integer.Integer.digits[30]>", line 1, in <module>
        n.digits(base=Integer(10))[-Integer(1)]  # slightly slower than str
        ^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "sage/rings/integer.pyx", line 1523, in sage.rings.integer.Integer.digits
        if do_sig_on: sig_on()
    SystemError: calling remove_from_pari_stack() inside sig_on()
**********************************************************************

random, but on half of the buildbots

@vbraun
Copy link
Member

vbraun commented Oct 28, 2023

Also get it without this, probably something else that randomly increases the failure probability.

vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 28, 2023
    
### 📚 Description

update pari to 2.15.4, drop patch

Will fix sagemath#35219

### 📝 Checklist

- [x] I have made sure that the title is self-explanatory and the
description concisely explains the PR.
    
URL: sagemath#35302
Reported by: Dima Pasechnik
Reviewer(s): Dima Pasechnik, Lorenz Panny, Michael Orlitzky
@dimpase
Copy link
Member Author

dimpase commented Oct 28, 2023

I recently removed sign_on/off from a piece of code which was calling plain Python. PR #??? (sorry, afk).
Perhaps it's more of the same thing.

@tornaria
Copy link
Contributor

FWIW, I've been on pari 2.15.3 since march and pari 2.15.4 since july, without any regressions.

Ofc these kind of bugs tend to appear on very heavy loaded boxes. The fact that now appears more often might also be related to the boxes now being more loaded.

@dimpase
Copy link
Member Author

dimpase commented Oct 28, 2023

this might be related to cypari2 memory model being very suboptimal, creating reference counted Python objects for everything Pari puts on the stack.

see #32360 (comment)

vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 29, 2023
    
### 📚 Description

update pari to 2.15.4, drop patch

Will fix sagemath#35219

### 📝 Checklist

- [x] I have made sure that the title is self-explanatory and the
description concisely explains the PR.
    
URL: sagemath#35302
Reported by: Dima Pasechnik
Reviewer(s): Dima Pasechnik, Lorenz Panny, Michael Orlitzky
@vbraun vbraun merged commit 9468290 into sagemath:develop Oct 31, 2023
23 of 34 checks passed
@jhpalmieri
Copy link
Member

jhpalmieri commented Nov 16, 2023

OS X does not have a command timeout, so that particular test (line 72 in spkg-configure.m4) fails and Sage refuses to use the system pari. The command

echo "f=factor(2^2203-1); print(\"ok\")" | gp -qf

finishes in well under 1 second on my machine, printing "ok":

% time echo "f=factor(2^2203-1); print(\"ok\")" | gp -qf
ok
echo "f=factor(2^2203-1); print(\"ok\")"  0.00s user 0.00s system 31% cpu 0.001 total
gp -qf  0.01s user 0.00s system 89% cpu 0.017 total

@jhpalmieri
Copy link
Member

@orlitzky
Copy link
Contributor

orlitzky commented Nov 17, 2023

OS X does not have a command timeout, so that particular test (line 72 in spkg-configure.m4) fails and Sage refuses to use the system pari.

We already have a version test near the top of pari's spkg-configure.m4. The additional tests for factor() and qfbclassno() are there because the main version test allows versions of pari older than 2.15.4. So, if you are extremely lucky/weird and if you have a pari-2.15.3 with two patches that fix bugs 2466 and 2469, then ./configure would still accept that pari-2.15.3.

I have always thought this was a waste of time, but since in this case it was someone else's time, who am I to complain :)

But what I'm getting at is that maybe now we should just bump the main version requirement up to 2.15.4. Anyone who would've packaged such a FrankenPARI-2.15.3 will most certainly have upgraded to 2.15.4 by now. Then the two bug tests (and in particular, the timeout) can be deleted.

@orlitzky
Copy link
Contributor

$ gp --version
                  GP/PARI CALCULATOR Version 2.15.4 (released)
$ grep -r PARI_VERSION_CODE /usr/include/pari/
/usr/include/pari/paricfg.h:#define PARI_VERSION_CODE 134916

@jhpalmieri
Copy link
Member

OS X does not have a command timeout, so that particular test (line 72 in spkg-configure.m4) fails and Sage refuses to use the system pari.

We already have a version test near the top of pari's spkg-configure.m4. The additional tests for factor() and qfbclassno() are there because the main version test allows versions of pari older than 2.15.4. So, if you are extremely lucky/weird and if you have a pari-2.15.3 with two patches that fix bugs 2466 and 2469, then ./configure would still accept that pari-2.15.3.

I have always thought this was a waste of time, but since in this case it was someone else's time, who am I to complain :)

But what I'm getting at is that maybe now we should just bump the main version requirement up to 2.15.4. Anyone who would've packaged such a FrankenPARI-2.15.3 will most certainly have upgraded to 2.15.4 by now. Then the two bug tests (and in particular, the timeout) can be deleted.

Or the logic could be: if you're using anything older than 2.15.4, run those particular tests, but skip those tests with 2.15.4 or later. Or at least do this with the one test that uses timeout.

I don't really have an opinion about how to solve it, I would just like a solution that lets me use homebrew's pari:

% brew info pari
==> pari: stable 2.15.4 (bottled)
...

and

% gp --version
                       GP/PARI CALCULATOR Version 2.15.4 (released)

@orlitzky
Copy link
Contributor

bumping the lower bound in #36732

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failure to handle large prime input with Pari's factor()
8 participants