Opened 2 years ago

Closed 8 months ago

Last modified 6 months ago

#23024 closed enhancement (fixed)

Replacement of sage/libs/ppl.pyx by pplpy

Reported by: vklein Owned by:
Priority: major Milestone: sage-8.7
Component: packages: standard Keywords: thursdaysbdx, ppl, pplpy, upgrade
Cc: vdelecroix, fbissey, embray, slelievre Merged in:
Authors: Vincent Klein Reviewers: Dima Pasechnik, François Bissey, Vincent Delecroix, Jeroen Demeyer, Erik Bray
Report Upstream: Reported upstream. Developers acknowledge bug. Work issues:
Branch: 829317e (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by slelievre)

The Sage wrappers around ppl are now superseded by the standalone pplpy (Python/Cython? compatible). This ticket proposes to replace src/sage/libs/ppl.pyx extension with pplpy. For this, we

  • create a new standard package pplpy
  • upgrade gmpy2 from optional to standard
  • replace the Sage code calling sage/libs/ppl.pyx to call pplpy
  • make appropriate deprecation for sage.libs.ppl

Tarball for pplpy: pplpy-0.8.4.tar.gz

Change History (217)

comment:1 Changed 2 years ago by vklein

  • Dependencies changed from #229 to #22928

comment:2 Changed 2 years ago by vklein

  • Type changed from PLEASE CHANGE to enhancement

comment:3 Changed 2 years ago by vdelecroix

  • Description modified (diff)

comment:4 Changed 2 years ago by vdelecroix

  • Description modified (diff)

comment:5 Changed 2 years ago by vklein

  • Branch set to u/vklein/replacement_of_sage_libs_ppl_pyx_by_pplpy

comment:6 Changed 2 years ago by git

  • Commit set to f8bf274d0a2477d621f7b8e88b79f2a23196d235

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

f8bf274Updating checksum for pplpy pkg

comment:7 Changed 2 years ago by vklein

For testing purpose you can use the following tarball of pplpy (outdated, use the one in the ticket's description)

Last edited 22 months ago by vklein (previous) (diff)

comment:8 Changed 2 years ago by vdelecroix

Salut Vincent,

Your branch appears in red which means that there are merge issues (possibly trivial). Would be better to rebase it on the latest beta.

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

1) Pickling should be fixed

-        sage: TestSuite(Asso).run()
+        sage: TestSuite(Asso).run(skip='_test_pickling')

It works with pplpy used from Python

>>> x = ppl.Variable(0)
>>> y = ppl.Variable(1)
>>> z = ppl.Variable(2)
>>> gs.insert(ppl.point(x+y+2*z))
>>> gs.insert(ppl.point(x-y))
>>> gs.insert(ppl.point(z))
>>> P = ppl.C_Polyhedron(gs)
>>> import pickle
>>> pickle.loads(pickle.dumps(P)) == P
True

2) I don't see the point of the function mpz_to_sage_integer. You can just do [Integer(x) for x in my_mpz_list]. For example

v_coords = (1,) + tuple(mpz_to_sage_integer(v.coefficients()))

can be replaced by

v_coords = (1,) + tuple(Integer(c) for c in v.coefficients())

Moreover, the name is confusing since the fuction converts a list of mpz integers.

3) Some possible simplifications

  • In ppl_backend.pyx:

Integer(g.coefficient(Variable(variable))) / Integer(g.divisor()) -> Rational(g.coefficient(Variable(variable)) / g.divisor())

  • In ppl_lattice_polytope.py: assert Integer(v.divisor()).is_one() -> assert v.divisor() == 1
Last edited 2 years ago by vdelecroix (previous) (diff)

comment:10 Changed 2 years ago by git

  • Commit changed from f8bf274d0a2477d621f7b8e88b79f2a23196d235 to dffa134588a520928393ec430bee91d0c2304fa2

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

f19f81eInteger and rational constructors support gmpy2 params
cd8ef5dremove OptionalExtension sage.libs.gmpy2
8ced5d3remove get_gmpy2_path()
674fe62Add few doctests
2ed4f95Use of MPZ_Check and MPQ_Check functions from gmpy2 C-API
b4f0c26Add pplpy as a standard package
257c330Replace sage.libs.ppl with pplpy package
636051fRemove sage.libs.ppl extension
f52b3d4Modify associahedron doctest
dffa134Updating checksum for pplpy pkg

comment:11 in reply to: ↑ 9 ; follow-up: Changed 2 years ago by vklein

Replying to vdelecroix:

3) Some possible simplifications

  • In ppl_backend.pyx:

Integer(g.coefficient(Variable(variable))) / Integer(g.divisor()) -> Rational(g.coefficient(Variable(variable)) / g.divisor())

  • In ppl_lattice_polytope.py: assert Integer(v.divisor()).is_one() -> assert v.divisor() == 1

It will be nice, but coercion for gmpy2 types is required for doing this (#23052).

comment:12 Changed 2 years ago by git

  • Commit changed from dffa134588a520928393ec430bee91d0c2304fa2 to b3563eb6a123d6aef327453061ea141e4fd1317c

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

b3563ebremove mpz_to_sage_integer function

comment:13 in reply to: ↑ 11 ; follow-up: Changed 2 years ago by vdelecroix

Replying to vklein:

Replying to vdelecroix:

3) Some possible simplifications

  • In ppl_backend.pyx:

Integer(g.coefficient(Variable(variable))) / Integer(g.divisor()) -> Rational(g.coefficient(Variable(variable)) / g.divisor())

  • In ppl_lattice_polytope.py: assert Integer(v.divisor()).is_one() -> assert v.divisor() == 1

It will be nice, but coercion for gmpy2 types is required for doing this (#23052).

Where? The first involves a direct call Rational(mpq_type) and the second comparison between mpz and Python integer. Which operation is mixing gmpy2 types with Sage types?

Last edited 2 years ago by vdelecroix (previous) (diff)

comment:14 in reply to: ↑ 13 Changed 2 years ago by vklein

Replying to vdelecroix:

Where? The first involves a direct call Rational(mpq_type) and the second comparison between mpz and Python integer. Which operation is mixing gmpy2 types with Sage types?

You're right, tests failures in ppl_backend.pyx with the suggested code are not coercion related. Forget my previous comment.

comment:15 Changed 2 years ago by git

  • Commit changed from b3563eb6a123d6aef327453061ea141e4fd1317c to d3875e7b6a4f8c87c24896259cc83a922c5c989a

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

3785bf4assert simplification ppl_lattice_polytope.py
d3875e7remove skip=('_test_pickling') option for pplpy polyhedron

comment:16 Changed 2 years ago by vklein

As pplpy has evolved to fix pickling with cPickle download pplpy again to update your local package.

Last edited 2 years ago by vklein (previous) (diff)

comment:17 Changed 2 years ago by git

  • Commit changed from d3875e7b6a4f8c87c24896259cc83a922c5c989a to 7ae21f5900110c74fdb8d925558d9f30282b9630

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

700f72cremove sage/libs/ppl from doctree
7ae21f5trac 23024 Bullet list indentation correction

comment:18 Changed 2 years ago by git

  • Commit changed from 7ae21f5900110c74fdb8d925558d9f30282b9630 to 858c2e5c6cdd8c3c85655d400c2cd535fc6ae855

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

08c7a72Add path of gmpy2 in include_dirs
359a0ffupdate checksum and package version
0d4fd2eInteger and rational constructors support gmpy2 params
7c66a7cremove OptionalExtension sage.libs.gmpy2
d143dderemove get_gmpy2_path()
2c56a3fAdd few doctests
e5264fdUse of MPZ_Check and MPQ_Check functions from gmpy2 C-API
504ab68Change way of creating a Rational from a mpz
072a15cadd __mpz__() to Rational
858c2e5trac 23024 update pplpy checksum

comment:19 Changed 2 years ago by vklein

As pplpy has evolved to fix pickling with cPickle download ​pplpy again to update your local package.

comment:20 Changed 2 years ago by vklein

  • Dependencies changed from #22928 to #22928, #23309

comment:21 Changed 2 years ago by git

  • Commit changed from 858c2e5c6cdd8c3c85655d400c2cd535fc6ae855 to 7a5812a16a76a78755784292ec2476dac990140d

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

1c1b971Replace sage.libs.ppl with pplpy package
2a1798eRemove sage.libs.ppl extension
d1b0b2fModify associahedron doctest
db21e1dUpdating checksum for pplpy pkg
eb215c3remove mpz_to_sage_integer function
53f30f3assert simplification ppl_lattice_polytope.py
f9a6415remove skip=('_test_pickling') option for pplpy polyhedron
924984cremove sage/libs/ppl from doctree
ce863ffupdate checksum, snapshot version update in setup.py
7a5812aAdd path of gmpy2 in include_dirs

comment:22 Changed 2 years ago by vklein

  • Dependencies changed from #22928, #23309 to #22928
  • Description modified (diff)

comment:23 Changed 2 years ago by vdelecroix

pplpy 0.7 is out

comment:24 Changed 2 years ago by vklein

  • Milestone changed from sage-8.0 to sage-8.2

comment:25 Changed 22 months ago by git

  • Commit changed from 7a5812a16a76a78755784292ec2476dac990140d to dc37f382de20f4ff5cba86cdd86eaf5100af8f22

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

760bbdfModify associahedron doctest
6d3345eUpdating checksum for pplpy pkg
85cb9fcremove mpz_to_sage_integer function
70177c8assert simplification ppl_lattice_polytope.py
b0858a2remove skip=('_test_pickling') option for pplpy polyhedron
94c7817remove sage/libs/ppl from doctree
67c8c2eupdate checksum, snapshot version update in setup.py
50d62ffAdd path of gmpy2 in include_dirs
375fda6Trac #23024 Fix checksum (pplpy), spkg-install and spkg-check
dc37f38Trac #23024 Add new mpz gmpy2 to sage Integer conversion

comment:26 Changed 22 months ago by vklein

Rebase on 22928 (with 22928 based on 8.2.beta0)

comment:27 Changed 22 months ago by vdelecroix

You need to provide a link to the pplpy tarball.

comment:28 Changed 22 months ago by vdelecroix

  • Description modified (diff)

And proper deprecation has to be done for sage.libs.ppl so that a user doing

sage: from sage.libs.ppl import whatever

should not get an ImportError but a warning. See http://doc.sagemath.org/html/en/reference/misc/sage/misc/superseded.htm

comment:29 Changed 22 months ago by vklein

  • Description modified (diff)

comment:30 follow-up: Changed 22 months ago by vklein

And by proper deprecation do you mean keeping sage.libs.ppl fully functioning with his doctests ? Or do we prefer to shutdown sage.libs.ppl functionalities (Functions don't do their old work and just return a warning) ?

comment:31 in reply to: ↑ 30 Changed 22 months ago by vdelecroix

Replying to vklein:

And by proper deprecation do you mean keeping sage.libs.ppl fully functioning with his doctests ? Or do we prefer to shutdown sage.libs.ppl functionalities (Functions don't do their old work and just return a warning) ?

The aim is not to keep sage.libs.ppl but to just to have some indirection in the import so that you do not break user's code. As sage.libs.ppl will not here anymore I do not expect the full doctesuite to pass as it used to be. But the following kind of things should work (and be doctested)

sage: from sage.libs.ppl import Variable, Constraint_System, C_Polyhedron
UserWarning: proper deprecation message
sage: x = Variable(0)
sage: y = Variable(1)
sage: cs = Constraint_System()
sage: cs.insert(x >= 0)
sage: cs.insert(x <= 3)
sage: cs.insert(y >= 0)
sage: cs.insert(y <= 3)
sage: poly_from_constraints = C_Polyhedron(cs)

And the various objects constructed above are of course from pplpy.

You can have a look at other places in the sage code and look for "deprecation" and "lazy_import".

comment:32 Changed 22 months ago by vklein

Well, i spoke with Sebastien L. and if the goal is to let one year for developers to adapt their old code, doing an indirection is not enough. Mainly because pplpy use gmpy2 (some functions return and calls are differents).

Therefore it seems like we should keep sage.libs.ppl old code with added warnings.

Last edited 22 months ago by vklein (previous) (diff)

comment:33 Changed 22 months ago by fbissey

  • Cc fbissey added

comment:34 Changed 22 months ago by fbissey

Just a small request can you put upstream's address in SPKG.txt? Of course you are available on pypi but it is not the same as being able to find where to fill bugs and what the current issues are straight away.

comment:35 Changed 22 months ago by git

  • Commit changed from dc37f382de20f4ff5cba86cdd86eaf5100af8f22 to 7d191546377db00ef950b8936895f58b48b25b85

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

7d19154Trac #23024 SPKG.txt add upstream contact and licence.

comment:36 Changed 22 months ago by vklein

Done, thanks for noticing that.

comment:37 Changed 22 months ago by git

  • Commit changed from 7d191546377db00ef950b8936895f58b48b25b85 to 01403bd74b0863cc11422a13f95051a84cb9a291

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

01403bdTrac #23024 Deprecate sage.libs.ppl

comment:38 follow-up: Changed 22 months ago by vklein

Deprecation of sage.libs.ppl added. As the whole module is deprecated i choose to send deprecation warning at import time.

comment:39 in reply to: ↑ 38 Changed 22 months ago by vdelecroix

Replying to vklein:

Deprecation of sage.libs.ppl added. As the whole module is deprecated i choose to send deprecation warning at import time.

Good solution.

Now, we just need to wait for #22928...

comment:40 Changed 22 months ago by vdelecroix

Did you run the testsuite!?

sage -t src/sage/geometry/lattice_polytope.py
**********************************************************************
File "src/sage/geometry/lattice_polytope.py", line 1608, in sage.geometry.lattice_polytope.LatticePolytopeClass.boundary_point_indices
Failed example:
    face.points()
Exception raised:
    Traceback (most recent call last):
      File "/home/vdelecro/sage_patchbot/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 517, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/vdelecro/sage_patchbot/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 920, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.geometry.lattice_polytope.LatticePolytopeClass.boundary_point_indices[4]>", line 1, in <module>
        face.points()
      File "/home/vdelecro/sage_patchbot/local/lib/python2.7/site-packages/sage/geometry/lattice_polytope.py", line 3680, in points
        l = gcd(v)
    NameError: global name 'gcd' is not defined

comment:41 Changed 22 months ago by git

  • Commit changed from 01403bd74b0863cc11422a13f95051a84cb9a291 to d244109bd7ed07fa1c3cbec2a08f68a2e2730a74

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

e395e0eTrac #23024 Use gmpy2 as a standard package
d244109Trac #23024 Restore import gcd

comment:42 Changed 22 months ago by vklein

Yes i have see this one. The last remaining problem is a SEGFAULT when running ppl_lattice_polytope.py doctests.

Last edited 22 months ago by vklein (previous) (diff)

comment:43 follow-up: Changed 22 months ago by vdelecroix

Could you post the log here?

comment:44 in reply to: ↑ 43 Changed 22 months ago by vklein

Replying to vdelecroix:

Could you post the log here?

segfault_ppl_lattice_polytope.log

Short story is

sage: LatticePolytope_PPL((0,0),(1/2,1))

cause a Segfault.

Last edited 22 months ago by vklein (previous) (diff)

comment:46 Changed 22 months ago by vdelecroix

At least with the patch applied I got

sage: from sage.geometry.polyhedron.ppl_lattice_polytope import LatticePolytope_PPL
sage: LatticePolytope_PPL((0,0),(1/2,1))
Traceback (most recent call last):
...
TypeError: rational is not an integer

comment:47 Changed 22 months ago by vdelecroix

See #24417

comment:48 in reply to: ↑ 45 Changed 22 months ago by vklein

Replying to vdelecroix:

Isn't it https://github.com/aleaxit/gmpy/pull/174?

Yes it is ! Nicely done.

comment:49 Changed 21 months ago by git

  • Commit changed from d244109bd7ed07fa1c3cbec2a08f68a2e2730a74 to 3cec63259adb968dc3b70d4514d21c3e5530842f

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

7596cffremove sage/libs/ppl from doctree
0cd326cupdate checksum, snapshot version update in setup.py
91b5d52Add path of gmpy2 in include_dirs
0b965a7Trac #23024 Fix checksum (pplpy), spkg-install and spkg-check
dd886fbTrac #23024 Add new mpz gmpy2 to sage Integer conversion
b7a5a21Trac #23024 SPKG.txt add upstream contact and licence.
96146caTrac #23024 Deprecate sage.libs.ppl
e85ae5fTrac #23024 Use gmpy2 as a standard package
da9aa35Trac #23024 Restore import gcd
3cec632Trac #23024 : merge #22928 ppl.pyx add deprecation warning ...

comment:50 Changed 21 months ago by git

  • Commit changed from 3cec63259adb968dc3b70d4514d21c3e5530842f to d206c1432ef3bd7f394921de43363fb4bdc84188

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

d206c14Trac #23024: ppl.pyx add deprecation warning to long doctest

comment:51 Changed 21 months ago by vdelecroix

You are adding sage.libs.gmpy2 in 7596cff and removing it in 91b5d52...

comment:52 Changed 21 months ago by vklein

Yes adding it was one of our old solutions. Before having a better include in pplpy's setup.py if i remember correctly. All of this historical things can be squashed later.

comment:53 Changed 21 months ago by vklein

It's the same for ppl.pyx we don't need to show that we deleted it and restore and deprecate later.

comment:54 Changed 21 months ago by vdelecroix

There are a lot of trailing whitespaces that should be removed.

comment:55 Changed 21 months ago by git

  • Commit changed from d206c1432ef3bd7f394921de43363fb4bdc84188 to f3e4bcd318536643b1b8a4bf6e3560813e0970bd

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

886424bTrac 23024: Add pplpy as a standard package
a6e1cc7Trac 23024: Replace sage.libs.ppl with pplpy package
f3e4bcdTrac #23024: Use gmpy2 as a standard package

comment:56 Changed 21 months ago by vklein

History rewrited. Trailing whitespaces removed in real_mpfr.pyx, complex_mpc.pyx and complex_number.pyx files.

Last edited 21 months ago by vklein (previous) (diff)

comment:57 Changed 21 months ago by vdelecroix

I don't understand why you are modifying pplpy/SPKG.txt, pplpy/checksums.ini, pplpy/spkg-check, etc in both 886424b and ​a6e1cc7.

comment:58 Changed 21 months ago by vdelecroix

Build fine and all tests pass on my computer!

comment:59 Changed 21 months ago by vklein

  • Description modified (diff)

comment:60 Changed 21 months ago by jdemeyer

Use sdh_pip_install instead of $PIP_INSTALL

comment:61 follow-up: Changed 21 months ago by jdemeyer

Is the license of pplpy really GPL version 3? Or do you mean GPL version 3 or later? The difference is important.

comment:62 follow-up: Changed 21 months ago by jdemeyer

This looks like a strange way to check if something is one:

Integer(p.divisor()).is_one()

What's wrong with

p.divisor() == 1

comment:63 in reply to: ↑ 61 ; follow-up: Changed 21 months ago by vklein

Replying to jdemeyer:

Is the license of pplpy really GPL version 3? Or do you mean GPL version 3 or later? The difference is important.

Yes the current one is GPL version 3 (see LICENSE.txt). Is it better to say GPL version 3 or later anyway ?

comment:64 Changed 21 months ago by jdemeyer

In backend_ppl.py you do from gmpy2 import mpz but you're not using it.

comment:65 in reply to: ↑ 63 Changed 21 months ago by jdemeyer

Replying to vklein:

Is it better to say GPL version 3 or later anyway ?

See https://github.com/videlec/pplpy/issues/28

comment:66 in reply to: ↑ 62 ; follow-up: Changed 21 months ago by vklein

Replying to jdemeyer:

What's wrong with

p.divisor() == 1

p.divisor() return a gmpy2.mpz.

comment:67 Changed 21 months ago by git

  • Commit changed from f3e4bcd318536643b1b8a4bf6e3560813e0970bd to ef97c325b862e31856dc63c127c979692d7d31f5

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

ef97c32Trac #23024: spkg-install `sdh_pip_install` instead ...

comment:68 in reply to: ↑ 66 ; follow-up: Changed 21 months ago by vdelecroix

Replying to vklein:

Replying to jdemeyer:

What's wrong with

p.divisor() == 1

p.divisor() return a gmpy2.mpz.

What is wrong with

>>> gmpy2.mpz(1) == 1
True

comment:69 in reply to: ↑ 68 Changed 21 months ago by vklein

Replying to vdelecroix:

... What is wrong with

>>> gmpy2.mpz(1) == 1
True


In this case you compare a python int with a gmpy2.mpz.

But with sage's Integer :

sage: gmpy2.mpz(1) == 1
False

Edit: Hum ok my bad.

Last edited 21 months ago by vklein (previous) (diff)

comment:70 Changed 21 months ago by git

  • Commit changed from ef97c325b862e31856dc63c127c979692d7d31f5 to 2f98d707835ee148592d341666babec1d79e1419

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

2f98d70Trac #23024: Code cleaning

comment:71 Changed 21 months ago by dimpase

self-testing fails:

$ SAGE_CHECK=yes ./sage -f pplpy
...
[pplpy-0.7] Successfully installed pplpy-0.7
[pplpy-0.7] Cleaning up...
[pplpy-0.7] 
[pplpy-0.7] real	0m38.345s
[pplpy-0.7] user	0m36.490s
[pplpy-0.7] sys	0m1.835s
[pplpy-0.7] Copying package files from temporary location /home/dima/Sage/sage-dev/local/var/tmp/sage/build/pplpy-0.7/inst to /home/dima/Sage/sage-dev/local
[pplpy-0.7] Running the test suite for pplpy-0.7...
[pplpy-0.7] running test
[pplpy-0.7] error: cannot copy tree './tests': not a directory
[pplpy-0.7] 

comment:73 Changed 20 months ago by embray

  • Cc embray added

comment:74 Changed 9 months ago by jdemeyer

  • Dependencies #22928 deleted

comment:75 Changed 9 months ago by embray

  • Milestone changed from sage-8.2 to sage-pending

This would still be a good thing to do IMO, in general.

comment:76 Changed 9 months ago by vdelecroix

  • Description modified (diff)

comment:77 Changed 9 months ago by vdelecroix

  • Description modified (diff)
  • Milestone changed from sage-pending to sage-8.7

comment:78 Changed 9 months ago by git

  • Commit changed from 2f98d707835ee148592d341666babec1d79e1419 to 55ad161c47d0a901cf9e81d2adeb09b94c66d655

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

f5b7110Trac 23024: Add pplpy as a standard package
82a5d24Trac 23024: Replace sage.libs.ppl with pplpy package
59617e1Trac #23024: Use gmpy2 as a standard package
8f51887Trac #23024: spkg-install `sdh_pip_install` instead ...
d7a67d4Trac #23024: Code cleaning
9ea9382Trac #23024: Remove HAVE_GMPY2 from coerce.pyx
55ad161Trac #23024: fix ppl_lattice_polytope doctests

comment:79 Changed 9 months ago by vklein

Rebased on sage8.6

comment:80 Changed 9 months ago by vklein

  • Description modified (diff)

comment:81 Changed 9 months ago by git

  • Commit changed from 55ad161c47d0a901cf9e81d2adeb09b94c66d655 to e90bc7781e0685485908bb781fb6ea78eb28ac10

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

90d999bTrac #23024: fix ppl_lattice_polytope doctests
e90bc77Trac #23024: upgrade pplpy version to 0.8.1

comment:82 Changed 9 months ago by vklein

  • Status changed from new to needs_review

upgrade pplpy to 0.8.1

comment:83 Changed 9 months ago by vdelecroix

Apparently a merge failure with sage 8.7.beta0

comment:84 Changed 9 months ago by git

  • Commit changed from e90bc7781e0685485908bb781fb6ea78eb28ac10 to 9a065413932ab5d11d86789ddf687b471f88f699

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

c37dcb0Trac 23024: Add pplpy as a standard package
3f5eed8Trac 23024: Replace sage.libs.ppl with pplpy package
26d3d54Trac #23024: Use gmpy2 as a standard package
1a5d7e5Trac #23024: spkg-install `sdh_pip_install` instead ...
62452d5Trac #23024: Code cleaning
58a0e8aTrac #23024: Remove HAVE_GMPY2 from coerce.pyx
dabd865Trac #23024: fix ppl_lattice_polytope doctests
9a06541Trac #23024: upgrade pplpy version to 0.8.1

comment:85 Changed 9 months ago by vklein

Solve merge failure with sage 8.7.beta0.

comment:86 Changed 9 months ago by dimpase

  • off-topic - Vincent, please provide info on

https://github.com/OpenDreamKit/OpenDreamKit/issues/281

comment:87 Changed 9 months ago by vdelecroix

  • Status changed from needs_review to needs_work

You should set gmpy2 and pplpy as dependencies of the Sage library (otherwise building the Cython code might fail). This is done in build/make/deps.

comment:88 Changed 9 months ago by vdelecroix

Why

-            sage: TestSuite(P).run(skip='_test_pickling')
+            sage: TestSuite(P).run(skip=('_test_pickling'))

comment:89 follow-up: Changed 9 months ago by vdelecroix

And

-    def _repr_(self):
+    def __repr__(self):

comment:90 Changed 9 months ago by vdelecroix

The links in the documentation :class:`ppl.C_Polyhedron`, :class:`~ppl.Polyhedron`, etc are broken. These should not be sphinx links at all.

comment:91 follow-up: Changed 9 months ago by dimpase

Does pplpy build/install its own docs? This kind of setup is used e.g. in cvxopt.

comment:92 in reply to: ↑ 91 Changed 9 months ago by vdelecroix

Replying to dimpase:

Does pplpy build/install its own docs? This kind of setup is used e.g. in cvxopt.

It does build documentation if you ask it to. Tough it does not install anything (perhaps it should?). But even with properly installed documentation, I don't see how to make linking work properly.

comment:93 follow-up: Changed 9 months ago by dimpase

Packages install docs into $SAGE_LOCAL/share/doc/. I guess that docs can be linked to docs in Sage code via relative paths, i.e. ../../..

comment:94 in reply to: ↑ 89 Changed 9 months ago by vklein

Replying to vdelecroix:

And

-    def _repr_(self):
+    def __repr__(self):

That's because LatticePolytope_PPL_class now inherit from pplpy's C_Polyhedron and therefore LatticePolytope_PPL_class is not a SageObject.

comment:95 in reply to: ↑ 93 ; follow-ups: Changed 9 months ago by vklein

Replying to dimpase:

Packages install docs into $SAGE_LOCAL/share/doc/. I guess that docs can be linked to docs in Sage code via relative paths, i.e. ../../..

I can try.
But i can see a potential problem here: most packages seems to install documentation on if [[ "$SAGE_SPKG_INSTALL_DOCS" = yes ]].
If we do the same for pplpy, given $SAGE_SPKG_INSTALL_DOCS default value is not yes that mean that the links will be broken by default.

I think there is many distinct questions :

  • 1. Should we install a pplpy documentation ?
  • 2. If yes should we install it only if "$SAGE_SPKG_INSTALL_DOCS" = yes ?
  • 3. If yes to 1. and yes to 2. should we link the sagemaths documentation with pplpy's.
Last edited 9 months ago by vklein (previous) (diff)

comment:96 Changed 9 months ago by git

  • Commit changed from 9a065413932ab5d11d86789ddf687b471f88f699 to 13507e4fc42743661147a4071ef7be95a3871c45

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

13507e4Trac #23024: Implement reviewer's comments : add depende...

comment:97 Changed 9 months ago by vdelecroix

It is a good practice to have the first line of the commit below 50 characters. More precisely, any commit message should be

one line with <= 50 characters
(optional) linebreak
(optional) more extensive paragraph with lines <= 72 characters

comment:98 in reply to: ↑ 95 Changed 9 months ago by vdelecroix

Replying to vklein:

Replying to dimpase:

Packages install docs into $SAGE_LOCAL/share/doc/. I guess that docs can be linked to docs in Sage code via relative paths, i.e. ../../..

I can try.
But i can see a potential problem here: most packages seems to install documentation on if [[ "$SAGE_SPKG_INSTALL_DOCS" = yes ]].
If we do the same for pplpy, given $SAGE_SPKG_INSTALL_DOCS default value is not yes that mean that the links will be broken by default.

I think there is many distinct questions :

  • 1. Should we install a pplpy documentation ?
  • 2. If yes should we install it only if "$SAGE_SPKG_INSTALL_DOCS" = yes ?
  • 3. If yes to 1. and yes to 2. should we link the sagemaths documentation with pplpy's.

I don't know what is the way to proceed. See https://gitlab.com/videlec/pplpy/issues/12

For now I would say:

comment:99 Changed 9 months ago by dimpase

I've left comments in https://gitlab.com/videlec/pplpy/issues/12 namely there appears to be a Sphinx extension to link different projects.

It's too early to start blindly removing links to docs, probably there is a more intelligent way to preserve what is already there.

comment:100 in reply to: ↑ 95 ; follow-up: Changed 9 months ago by vdelecroix

Replying to vklein:

Replying to dimpase:

Packages install docs into $SAGE_LOCAL/share/doc/. I guess that docs can be linked to docs in Sage code via relative paths, i.e. ../../..

I can try.
But i can see a potential problem here: most packages seems to install documentation on if [[ "$SAGE_SPKG_INSTALL_DOCS" = yes ]].
If we do the same for pplpy, given $SAGE_SPKG_INSTALL_DOCS default value is not yes that mean that the links will be broken by default.

I think there is many distinct questions :

  • 1. Should we install a pplpy documentation ?

We should compile and install.

  • 2. If yes should we install it only if "$SAGE_SPKG_INSTALL_DOCS" = yes ?

I don't know. Where is SAGE_SPKG_INSTALL_DOCS documented?

  • 3. If yes to 1. and yes to 2. should we link the sagemaths documentation with pplpy's.

See #27164.

comment:101 in reply to: ↑ 100 ; follow-up: Changed 9 months ago by vklein

Replying to vdelecroix:

...

  • 2. If yes should we install it only if "$SAGE_SPKG_INSTALL_DOCS" = yes ?

I don't know. Where is SAGE_SPKG_INSTALL_DOCS documented?

Here http://doc.sagemath.org/html/en/installation/source.html.

SAGE_SPKG_INSTALL_DOCS - if set to yes, then install package-specific documentation to $SAGE_ROOT/local/share/doc/PACKAGE_NAME/ when an spkg is installed. This option may not be supported by all spkgs. Some spkgs might also assume that certain programs are available on the system (for example, latex or pdflatex).

comment:102 Changed 9 months ago by git

  • Commit changed from 13507e4fc42743661147a4071ef7be95a3871c45 to 665419ad8dc012687220cb40ad6c09a943ca9b44

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

665419aTrac #23024: Implement reviewer's comments ...

comment:103 in reply to: ↑ 101 Changed 9 months ago by vdelecroix

Replying to vklein:

Replying to vdelecroix:

...

  • 2. If yes should we install it only if "$SAGE_SPKG_INSTALL_DOCS" = yes ?

I don't know. Where is SAGE_SPKG_INSTALL_DOCS documented?

Here http://doc.sagemath.org/html/en/installation/source.html.

SAGE_SPKG_INSTALL_DOCS - if set to yes, then install package-specific documentation to $SAGE_ROOT/local/share/doc/PACKAGE_NAME/ when an spkg is installed. This option may not be supported by all spkgs. Some spkgs might also assume that certain programs are available on the system (for example, latex or pdflatex).

I believe it has been done that way a long time ago when Sage was not thought modular at all. Some of the package documentation (including pplpy) might be automatically installed unless SAGE_SPKG_INSTALL_DOCS is explicitely set to "no".

comment:104 Changed 9 months ago by git

  • Commit changed from 665419ad8dc012687220cb40ad6c09a943ca9b44 to ef8997834f73c34d983260c0ba782041d0966a02

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

ef89978Trac #23024: Generate and installl doc

comment:105 Changed 9 months ago by vklein

For testing purpose use https://www.labri.fr/perso/vklein/pplpy-0.8.2b0.tar.gz with ef89978.

comment:106 Changed 9 months ago by vklein

  • Description modified (diff)

ef89978 Implements the compilation and installation of pplpy's documentation.

The current implementation drawback is that pplpy is compiled two times (one time for package installation and one time using "inplace" build in preparation for sphinx).

To avoid that i am looking for an elegant and generic way to express PYTHONPATH=$PYTHONPATH:$SAGE_DESTDIR_LOCAL/lib/python2.7/site-packages.

I haven't find an environment variable for lib/python2.7/site-packages or something equivalent.

Any help is welcomed.

Last edited 9 months ago by vklein (previous) (diff)

comment:107 Changed 9 months ago by git

  • Commit changed from ef8997834f73c34d983260c0ba782041d0966a02 to d8c41f665958133b323380f70800a66c6432820a

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

d8c41f6Trac #23024: Add intersphinx between sage doc ...

comment:108 Changed 9 months ago by vklein

d8c41f6 Fix intersphinx question.

I wait for returns for #comment:106 and for a new pplpy official version before setting this ticket in need_review.

comment:109 follow-up: Changed 9 months ago by dimpase

I wonder whether you tried to set up a link to pplpy docs in Sage's reference manual, something that ought to be done for many more packages...

comment:110 in reply to: ↑ 109 Changed 9 months ago by vklein

Replying to dimpase:

I wonder whether you tried to set up a link to pplpy docs in Sage's reference manual, something that ought to be done for many more packages...


I am not sure what is your question about.

I have think about doing the intersphinx link at reference level yes. But IMHO doing it under common is a better call.

I haven't tried to do direct html link with relative path.

comment:111 follow-up: Changed 9 months ago by dimpase

I meant to ask whether you managed to replace the docs link to PPL backend in the reference manual, such as http://doc.sagemath.org/html/en/reference/discrete_geometry/sage/geometry/polyhedron/backend_ppl.html (its local equivalent would have something else instead of http://doc.sagemath.org/ part, naturally) with the link to the pplpy docs.

comment:112 follow-up: Changed 9 months ago by vdelecroix

Crossed links does not work for me (e.g. the ppl.C_Polyhedron in the file en/reference/discrete_geometry/sage/geometry/polyhedron/ppl_lattice_polytope.html) ... did you do something special in order to make it work for you?

comment:113 in reply to: ↑ 112 ; follow-up: Changed 9 months ago by vklein

Replying to vdelecroix:

Crossed links does not work for me (e.g. the ppl.C_Polyhedron in the file en/reference/discrete_geometry/sage/geometry/polyhedron/ppl_lattice_polytope.html) ... did you do something special in order to make it work for you?

Maybe try a make doc-clean. `ppl.C_Polyhedron` should have been replaced by `ppl.polyhedron.C_Polyhedron`.

comment:114 in reply to: ↑ 113 Changed 9 months ago by vdelecroix

Replying to vklein:

Replying to vdelecroix:

Crossed links does not work for me (e.g. the ppl.C_Polyhedron in the file en/reference/discrete_geometry/sage/geometry/polyhedron/ppl_lattice_polytope.html) ... did you do something special in order to make it work for you?

Maybe try a make doc-clean. `ppl.C_Polyhedron` should have been replaced by `ppl.polyhedron.C_Polyhedron`.

I did make doc-clean, sage -f pplpy and make doc.

comment:115 Changed 9 months ago by vdelecroix

I know what I did wrong: I did not pull your last commits... sorry for the noise.

comment:116 Changed 9 months ago by vdelecroix

It is working now!

comment:117 Changed 9 months ago by vdelecroix

I will make a new release of pplpy.

Could you ask about 106 on sage-devel? This is a general problem when a package needs to be installed before being able to build the docs. Perhaps this is a problem with the package itself or in the way our current staged installation works in Sage. But then, it is not clear to me how to solve it.

comment:119 in reply to: ↑ 111 Changed 9 months ago by vklein

Replying to dimpase:

I meant to ask whether you managed to replace the docs link to PPL backend in the reference manual, such as http://doc.sagemath.org/html/en/reference/discrete_geometry/sage/geometry/polyhedron/backend_ppl.html (its local equivalent would have something else instead of http://doc.sagemath.org/ part, naturally) with the link to the pplpy docs.

I managed to replace the doc links of type sage.libs.ppl.*. But there is no such kind of doclinks under backend_ppl.html. Class like sage.geometry.polyhedron.base_QQ.Polyhedron_QQ are still sage classes.

comment:120 follow-ups: Changed 9 months ago by vdelecroix

Alternative: include the compiled doc in the source tarball so that the spkg-install script just need to copy it...

comment:121 in reply to: ↑ 120 ; follow-up: Changed 9 months ago by dimpase

Replying to vdelecroix:

Alternative: include the compiled doc in the source tarball so that the spkg-install script just need to copy it...

-1. I'd rather run the installation twice than this.

comment:122 in reply to: ↑ 120 ; follow-up: Changed 9 months ago by vklein

Replying to vdelecroix:

Alternative: include the compiled doc in the source tarball so that the spkg-install script just need to copy it...

Why not. It will increase the tarball size of something like 200 kbytes.

comment:123 in reply to: ↑ 121 Changed 9 months ago by vklein

Replying to dimpase:

Replying to vdelecroix:

Alternative: include the compiled doc in the source tarball so that the spkg-install script just need to copy it...

-1. I'd rather run the installation twice than this.

Why ? (I personally have no preferences between the two solutions).

Last edited 9 months ago by vklein (previous) (diff)

comment:124 in reply to: ↑ 122 Changed 9 months ago by dimpase

Replying to vklein:

Replying to vdelecroix:

Alternative: include the compiled doc in the source tarball so that the spkg-install script just need to copy it...

Why not. It will increase the tarball size of something like 200 kbytes.

The external tarball can contain extra 200K, sure, but already "just copying" has to be done properly, in order not to break package de-installation. Besides, there is no guarantee it would look nice enough, so that it matches the current Sage docs style, etc.

comment:125 Changed 9 months ago by vklein

Ok i see. And i agree that having the same sphinx install generating both documentation is a good thing.

comment:126 Changed 9 months ago by git

  • Commit changed from d8c41f665958133b323380f70800a66c6432820a to c40f656f7f605435339e33a3ed1d4ac8598852fb

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

41e16e6Trac #23024: Use gmpy2 as a standard package
68e0e45Trac #23024: spkg-install `sdh_pip_install` instead ...
9da630fTrac #23024: Code cleaning
273b772Trac #23024: Remove HAVE_GMPY2 from coerce.pyx
1365debTrac #23024: fix ppl_lattice_polytope doctests
3da5acfTrac #23024: upgrade pplpy version to 0.8.1
9fed2a3Trac #23024: Implement reviewer's comments ...
33f4759Trac #23024: Generate and installl doc
3cf578cTrac #23024: Add intersphinx between sage doc ...
c40f656Trac #23024: Upgrade to pplpy version 8.2

comment:127 Changed 9 months ago by vklein

  • Description modified (diff)

Upgrade pplpy to 8.2.
Rebase on sage 8.7.beta1.

For the doc generation, i think we can go with the double compilation now and switch
to a better solution later if we find one.

comment:128 Changed 9 months ago by git

  • Commit changed from c40f656f7f605435339e33a3ed1d4ac8598852fb to 93e0b66d6d76cdb867ec049a7be12c123a2ba769

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

bcb1373Trac #23024: Add pplpy as a standard package
1aec143Trac #23024: Replace sage.libs.ppl with pplpy package
3231e47Trac #23024: Use gmpy2 as a standard package
93e0b66Trac #23024: Doc and dependencies: ...

comment:129 Changed 9 months ago by vklein

  • Status changed from needs_work to needs_review

Rewrite ticket history.

comment:130 follow-up: Changed 9 months ago by vdelecroix

To my mind, a better solution has to be found for

+    # Compile pplpy's documentation
+    sage-python23 setup.py build_ext --inplace
+    PYTHONPATH=$PYTHONPATH:$(pwd)
+    cd docs
+    $MAKE html

comment:131 Changed 9 months ago by dimpase

As long as gmpy2 is optional, this package cannot be standard, as it depends on gmpy2.

(Edit - I misread the ticket description, hence the edit). I recall last time there was a discussion about gmpy2 being standard, there were quite a bit of objections. Do they still stand?

Last edited 9 months ago by dimpase (previous) (diff)

comment:132 Changed 9 months ago by vklein

  • Status changed from needs_review to needs_work

Yes, this is the discussion link https://groups.google.com/forum/?#!topic/sage-devel/qoxVng1__0A.

The discussion/objections where more about what types pplpy should return (Sage numbers or gmpy2 numbers).

The last 3 messages for gmpy2 solution have never been answered, but rereading the thread it's true that it doesn't mean that we have an explicit consensus.

I put this ticket on hold in wait for better solution for generating documentation.

I will repost on the discussion thread when this ticket is ready.

comment:133 in reply to: ↑ 130 Changed 9 months ago by dimpase

Replying to vdelecroix:

To my mind, a better solution has to be found for

+    # Compile pplpy's documentation
+    sage-python23 setup.py build_ext --inplace
+    PYTHONPATH=$PYTHONPATH:$(pwd)
+    cd docs
+    $MAKE html

the question is - are you prepared to patch pplpy sources? Comparing their docs/Makefile with https://github.com/sagemath/sagenb/blob/master/doc/Makefile seems to say that there are sphinx options one might try to use to do everything in one build, not in two.

comment:134 Changed 9 months ago by vklein

I haven't find a decisive difference between the two Makefile. The most obvious one being that pplpy use sphinx-build -b and sagenb use sphinx-build -M.

I have tried with -M and get the same result.
I think the reason it works with sagenb is because it doesn't have cython files and therefore the source directory is enough for the sphinx build to succeed.

Last edited 9 months ago by vklein (previous) (diff)

comment:135 Changed 8 months ago by git

  • Commit changed from 93e0b66d6d76cdb867ec049a7be12c123a2ba769 to 8d020eac9061705ebe900a4be3ec1b3c66d1eb49

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

e2d3e07Trac #23024: Add pplpy as a standard package
ba656aeTrac #23024: Replace sage.libs.ppl with pplpy package
5ac4d45Trac #23024: Use gmpy2 as a standard package
c4a7255Trac #23024: Doc and dependencies: ...
8d020eaTrac #23024: Sphinx doc avoid to compile two ...

comment:136 Changed 8 months ago by vklein

Add a workaround to avoid two build and rebase on 8.7.beta3.

comment:137 Changed 8 months ago by vdelecroix

It is not working for me

[pplpy-0.8.2] WARNING: autodoc: failed to import module u'ppl'; the following exception was raised:
[pplpy-0.8.2] No module named ppl
[pplpy-0.8.2] WARNING: autodoc: failed to import module u'ppl.constraint'; the following exception was raised:
[pplpy-0.8.2] No module named ppl.constraint
[pplpy-0.8.2] WARNING: autodoc: failed to import module u'ppl.linear_algebra'; the following exception was raised:
[pplpy-0.8.2] No module named ppl.linear_algebra
[pplpy-0.8.2] WARNING: autodoc: failed to import module u'ppl.generator'; the following exception was raised:
[pplpy-0.8.2] No module named ppl.generator
[pplpy-0.8.2] WARNING: autodoc: failed to import module u'ppl.polyhedron'; the following exception was raised:
[pplpy-0.8.2] No module named ppl.polyhedron
[pplpy-0.8.2] WARNING: autodoc: failed to import module u'ppl.mip_problem'; the following exception was raised:
[pplpy-0.8.2] No module named ppl.mip_problem

comment:138 follow-up: Changed 8 months ago by vdelecroix

And shouldn't python be python23?

comment:139 Changed 8 months ago by vdelecroix

And the command you provided is very suspicious to me. Inside the sage shell

$ python -c "import site; print(site.getusersitepackages().replace(site.getuserbase(), ''))"
/home/vincent/.sage/local/lib/python2.7/site-packages

And using string replace is not an option

>>> import site
>>> site.getusersitepackages()
'/home/vincent/.sage/local/lib/python2.7/site-packages'
>>> site.getuserbase()
'/home/vincent/.sage//local'

comment:140 Changed 8 months ago by vdelecroix

This already looks more reasonable

>>> import sysconfig
>>> sysconfig.get_paths(expand=False)
{'purelib': '{base}/lib/python{py_version_short}/site-packages',
 'stdlib': '{base}/lib/python{py_version_short}',
 'scripts': '{base}/bin',
 'platinclude': '{platbase}/include/python{py_version_short}',
 'include': '{base}/include/python{py_version_short}',
 'data': '{base}', 'platstdlib':
 '{platbase}/lib/python{py_version_short}',
 'platlib': '{platbase}/lib/python{py_version_short}/site-packages'
}

comment:141 Changed 8 months ago by vdelecroix

I believe that what you want is

>>> import sysconfig
>>> py_version_short = sysconfig.get_config_vars()['py_version_short']
>>> path = sysconfig.get_path('platlib', expand=False)
>>> path.format(platbase="$SAGE_DESTDIR_LOCAL", py_version_short=py_version_short)
'$SAGE_DESTDIR_LOCAL/lib/python2.7/site-packages'

comment:142 Changed 8 months ago by vklein

the site module is already supposed to do the job in a generic way :

def _get_path(userbase):
    version = sys.version_info

    if os.name == 'nt':
        return f'{userbase}\\Python{version[0]}{version[1]}\\site-packages'

    if sys.platform == 'darwin' and sys._framework:
        return f'{userbase}/lib/python/site-packages'

    return f'{userbase}/lib/python{version[0]}.{version[1]}/site-packages'

Python version is not always used

comment:143 Changed 8 months ago by git

  • Commit changed from 8d020eac9061705ebe900a4be3ec1b3c66d1eb49 to 4b462ab0e597aba8d08286b5fdfd97855b9f8e8a

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

4b462abTrac #23024: Sphinx doc avoid to compile two ...

comment:144 in reply to: ↑ 138 Changed 8 months ago by vklein

Replying to vdelecroix:

And shouldn't python be python23?

Yes you are right.
Workaround fixed and enhanced with Erik Bray's help.

comment:145 Changed 8 months ago by vdelecroix

Same as before

[pplpy-0.8.2] WARNING: autodoc: failed to import module u'ppl'; the following exception was raised:
[pplpy-0.8.2] No module named ppl
[pplpy-0.8.2] WARNING: autodoc: failed to import module u'ppl.constraint'; the following exception was raised:
[pplpy-0.8.2] No module named ppl.constraint
[pplpy-0.8.2] WARNING: autodoc: failed to import module u'ppl.linear_algebra'; the following exception was raised:
[pplpy-0.8.2] No module named ppl.linear_algebra
[pplpy-0.8.2] WARNING: autodoc: failed to import module u'ppl.generator'; the following exception was raised:
[pplpy-0.8.2] No module named ppl.generator
[pplpy-0.8.2] WARNING: autodoc: failed to import module u'ppl.polyhedron'; the following exception was raised:
[pplpy-0.8.2] No module named ppl.polyhedron
[pplpy-0.8.2] WARNING: autodoc: failed to import module u'ppl.mip_problem'; the following exception was raised:
[pplpy-0.8.2] No module named ppl.mip_problem

comment:146 Changed 8 months ago by vklein

Can you post or mail me the whole log ?

comment:147 Changed 8 months ago by vklein

Ok i have an obvious bug in my spgk_install file

comment:148 Changed 8 months ago by git

  • Commit changed from 4b462ab0e597aba8d08286b5fdfd97855b9f8e8a to f4fe72be6c56056edf06850640459b2b1986bcea

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

f4fe72bTrac #23024: Sphinx doc avoid to compile two ...

comment:149 Changed 8 months ago by vklein

It's fixed sorry for the previous commit.

comment:150 Changed 8 months ago by vdelecroix

Why does sphinx inventory connect to internet!?

[pplpy-0.8.2] loading intersphinx inventory from http://docs.python.org/objects.inv...

comment:151 Changed 8 months ago by vklein

I believe because of pplpy's conf.py last line :

# Example configuration for intersphinx: refer to the Python standard library.
intersphinx_mapping = {'http://docs.python.org/': None}

This says pplpy's intersphinx needs python's objects.inv.

Wich isn't true if i am not wrong ( I don't remember a sphinx link to python documentation in pplpy's documentation)

Last edited 8 months ago by vklein (previous) (diff)

comment:152 Changed 8 months ago by vdelecroix

  • Description modified (diff)

new tarball ready

comment:153 Changed 8 months ago by git

  • Commit changed from f4fe72be6c56056edf06850640459b2b1986bcea to abaef5ef92cf1d2148602527c350957c76ef7e14

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

abaef5eTrac #23024: upgrade pplpy to version 0.8.3

comment:154 follow-up: Changed 8 months ago by vklein

  • Status changed from needs_work to needs_review

Ticket ready for review.

Don't set this ticket to positive review right now.
We need to clarify the discussion https://groups.google.com/forum/?#!topic/sage-devel/qoxVng1__0A first.

comment:155 in reply to: ↑ 154 Changed 8 months ago by vdelecroix

Replying to vklein:

Don't set this ticket to positive review right now.
We need to clarify the discussion https://groups.google.com/forum/?#!topic/sage-devel/qoxVng1__0A first.

What is the status of the discussion now?

comment:156 follow-up: Changed 8 months ago by vdelecroix

Note: for the documentation build, there is also the possibility to use the spkg-postinst script (that is run after the installation in $SAGE_LOCAL).

comment:157 in reply to: ↑ 156 ; follow-up: Changed 8 months ago by vklein

Replying to vdelecroix:

Note: for the documentation build, there is also the possibility to use the spkg-postinst script (that is run after the installation in $SAGE_LOCAL).

Yes that's true. The reason to keep the documentation build in spkg-inst is because it's currently the standard way of doing it. Tell me what you prefer.

comment:158 in reply to: ↑ 157 Changed 8 months ago by vdelecroix

Replying to vklein:

Replying to vdelecroix:

Note: for the documentation build, there is also the possibility to use the spkg-postinst script (that is run after the installation in $SAGE_LOCAL).

Yes that's true. The reason to keep the documentation build in spkg-inst is because it's currently the standard way of doing it. Tell me what you prefer.

To my knowledge, no package had this problem before (ie building the doc depending on the library being installed). Everything that would avoid modifying PYTHONPATH looks nicer to me. But that is my own feeling.

comment:159 Changed 8 months ago by git

  • Commit changed from abaef5ef92cf1d2148602527c350957c76ef7e14 to c6d75a9b610220d7e882a2be9f83591f81fbc480

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

c6d75a9Trac #23024: Add a spkg-postinst script to man...

comment:160 Changed 8 months ago by vklein

Ok seems fair. spkg-postinst script added.

comment:161 Changed 8 months ago by vdelecroix

Having a comment in the spkg-install script is good but I think that it would be even nicer with an explanation of why this is moved inside the spkg-postinst script.

comment:162 Changed 8 months ago by git

  • Commit changed from c6d75a9b610220d7e882a2be9f83591f81fbc480 to 794aed8c6a61b9397167c7cfe8c2925ae4e90370

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

794aed8Trac #23024: spkg-install: Document the reason ...

comment:163 Changed 8 months ago by vklein

Ok, explanation added.

comment:164 Changed 8 months ago by vdelecroix

The current status is good for me. Thanks Vincent.

Any comment Dima?

comment:165 Changed 8 months ago by dimpase

How about src/sage/numerical/backends/ppl_backend.pyx ? Do you at least have a ticket to work on replacing this with pplpy?

comment:166 Changed 8 months ago by vklein

ppl_backend.pyx is modified in this ticket. This is not replaced but ppl_backend.pyx methods use pplpy instead of using sage.lib.ppl.

see: https://git.sagemath.org/sage.git/diff/src/sage/numerical/backends/ppl_backend.pyx?id=fed0a135795f8f076d7ffaf69aecc84f2f096b5d

comment:167 Changed 8 months ago by vklein

@Dima tell me if you think it's ok or if you think ppl_backend.pyx should be reworked in another way.

comment:168 follow-up: Changed 8 months ago by dimpase

I recall that ppl_backend.pyx had a tricky part due to the inability of PPL to work with rationals - everything had to be integer. I don't know whether this is still the case. Potentially, if it, or/and pplpy can now work directly with rationals, it might be worthwhile to change, as computing common denominators lead to blowup of coefficients.

comment:169 in reply to: ↑ 168 ; follow-up: Changed 8 months ago by vdelecroix

Replying to dimpase:

I recall that ppl_backend.pyx had a tricky part due to the inability of PPL to work with rationals - everything had to be integer. I don't know whether this is still the case. Potentially, if it, or/and pplpy can now work directly with rationals, it might be worthwhile to change, as computing common denominators lead to blowup of coefficients.

The PPL Coefficient type is hardcoded in the Cython wrapper as

ctypedef mpz_class PPL_Coefficient

This implies that all computations with pplpy are done with GMP mpz. I agree that PPL supports computations with mpz_class, mpq_class as well as various native integer types (from char to long long). Nothing has changed on this side yet. This feature request is disjoint from this ticket which aims to be a simple transition from Sage source code to standalone library.

comment:170 Changed 8 months ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

looks good to me then.

comment:171 in reply to: ↑ 169 Changed 8 months ago by vdelecroix

Replying to vdelecroix:

Replying to dimpase:

I recall that ppl_backend.pyx had a tricky part due to the inability of PPL to work with rationals - everything had to be integer. I don't know whether this is still the case. Potentially, if it, or/and pplpy can now work directly with rationals, it might be worthwhile to change, as computing common denominators lead to blowup of coefficients.

The PPL Coefficient type is hardcoded in the Cython wrapper as

ctypedef mpz_class PPL_Coefficient

This implies that all computations with pplpy are done with GMP mpz. I agree that PPL supports computations with mpz_class, mpq_class as well as various native integer types (from char to long long). Nothing has changed on this side yet. This feature request is disjoint from this ticket which aims to be a simple transition from Sage source code to standalone library.

This is high priority and I opened an issue: https://gitlab.com/videlec/pplpy/issues/14

comment:172 Changed 8 months ago by vklein

  • Reviewers changed from Dima Pasechnik to Dima Pasechnik, François Bissey, ​Vincent Delecroix, Jeroen Demeyer

Thanks to all reviewers.

comment:173 Changed 8 months ago by vbraun

  • Status changed from positive_review to needs_work

merge conflict

comment:174 Changed 8 months ago by git

  • Commit changed from 794aed8c6a61b9397167c7cfe8c2925ae4e90370 to 81bf994a34c7f862cfb71d3a76ab4632017607ae

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

477a7beTrac #23024: Add pplpy as a standard package
44b61c5Trac #23024: Replace sage.libs.ppl with pplpy package
a4c722cTrac #23024: Use gmpy2 as a standard package
684e107Trac #23024: Doc and dependencies: ...
b49007cTrac #23024: Sphinx doc avoid to compile two ...
bfc737aTrac #23024: upgrade pplpy to version 0.8.3
b7e3547Trac #23024: Add a spkg-postinst script to man...
81bf994Trac #23024: spkg-install: Document the reason ...

comment:175 Changed 8 months ago by vklein

Rebase on 8.7.beta5

comment:176 Changed 8 months ago by vklein

  • Status changed from needs_work to needs_review

comment:177 Changed 8 months ago by vdelecroix

  • Status changed from needs_review to positive_review

comment:178 follow-ups: Changed 8 months ago by jdemeyer

  • Status changed from positive_review to needs_work

Looking at the diff, I have several minor comments:

  1. Don't change whitespace in src/module_list.py for no good reason.
  1. In various places, you use list(...). Why not use a list comprehension instead?
  1. In multi-line imports, the preferred style is to use parentheses instead of \.
  1. The deprecation message is really strangely formatted: it shouldn't start with a newline and there shouldn't be a huge amount of indentation in the second line. I would probably write it all on one line without any newlines at all.
  1. What's the meaning of the /... at the start of the deprecation warnings in doctests? I have never seen that.
  1. In _add_rational_constraint, why do you cdef Rational rhs? Since numerator and denominator are pure Python methods, this is a pessimization.

comment:179 in reply to: ↑ 178 Changed 8 months ago by embray

Replying to jdemeyer:

Looking at the diff, I have several minor comments:

  1. In various places, you use list(...). Why not use a list comprehension instead?

I wonder if there shouldn't be something like this mentioned in the developer docs (do we have a style guide)?

This is a comment I've made in review on dozens of tickets at this point.

Short list comprehensions (it's trickier when they get nested) are often clearer than any alternative, and are usually much more efficient from the Python interpreter's standpoint as well.

comment:180 in reply to: ↑ 178 Changed 8 months ago by vklein

  1. What's the meaning of the /... at the start of the deprecation warnings in doctests? I have never seen that.

It means the line begin with / then something before DeprecationWarning.... I think i have done that because the print(err) result begin by the path $SAGE_LOCAL/src/bin/sage-eval.

comment:181 Changed 8 months ago by jdemeyer

I see now. Those aren't "normal" doctests, they are actually run in a subprocess.

comment:182 Changed 8 months ago by git

  • Commit changed from 81bf994a34c7f862cfb71d3a76ab4632017607ae to b25b7199b237e4e732fc805ce56e40cf477f7e6d

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

b25b719Trac #23024: Implements reviewer's comment :

comment:183 Changed 8 months ago by vklein

  • Status changed from needs_work to needs_review

Comments 1,2,3,4 and 6 implemented.

comment:184 Changed 8 months ago by vklein

  • Priority changed from minor to major

comment:185 Changed 8 months ago by vdelecroix

Jeroen?

comment:186 follow-up: Changed 8 months ago by jdemeyer

  • Status changed from needs_review to positive_review

Good for me if this passes doctests (I haven't checked)

comment:187 in reply to: ↑ 186 Changed 8 months ago by vdelecroix

Replying to jdemeyer:

Good for me if this passes doctests (I haven't checked)

This I did (and I belive Vincent K also)

comment:188 Changed 8 months ago by vklein

After the last commit i only did tests on all modified files and everything under sage/geometry. I will launch a sage -t -a --long just to be extra sure.

comment:189 Changed 8 months ago by embray

  • Status changed from positive_review to needs_review

I would like to test this on Windows. I can do that in a few minutes.

comment:190 Changed 8 months ago by embray

  • Owner changed from (none) to embray

comment:191 Changed 8 months ago by embray

One minor comment just looking at the patch: To complement the pplpy/spkg-postinst script, it would be good, albeit not strictly necessary, to include a pplpy/spkg-postrm script that removes that documentation directory in its entirety, if it was installed. That way it's a bit tidier if you also remove or upgrade pplpy and its documentation changes.

comment:192 follow-up: Changed 8 months ago by vklein

Ok, when exactly are spkg-postrm scripts called ? sage -f pkgname after the uninstall ?

Anyways i will wait for Owner field to be empty before pushing any new commit.

Last edited 8 months ago by vklein (previous) (diff)

comment:193 Changed 8 months ago by embray

Yeah, pplpy doesn't build on Cygwin:

[pplpy-0.8.3]     g++ -shared -Wl,--enable-auto-image-base -L/home/embray/src/sagemath/sage/local/lib -Wl,-rpath,/home/embray/src/sagemath/sage/local/lib -L/home/embray/src/sagemath/sage/local/lib -Wl,-rpath,/home/embray/src/sagemath/sage/local/lib build/temp.cygwin-2.9.0-x86_64-2.7/ppl/bit_arrays.o build/temp.cygwin-2.9.0-x86_64-2.7/ppl/ppl_shim.o -L/home/embray/src/sagemath/sage/local/lib/python2.7/config -L/home/embray/src/sagemath/sage/local/lib -lppl -lm -lpython2.7 -o build/lib.cygwin-2.9.0-x86_64-2.7/ppl/bit_arrays.dll
[pplpy-0.8.3]     build/temp.cygwin-2.9.0-x86_64-2.7/ppl/bit_arrays.o: In function `Parma_Polyhedra_Library::Bit_Row::clear()':
[pplpy-0.8.3]     /home/embray/src/sagemath/sage/local/include/ppl.hh:40661: undefined reference to `__imp___gmpz_set_ui'
[pplpy-0.8.3]     /home/embray/src/sagemath/sage/local/include/ppl.hh:40661:(.text+0x14c): relocation truncated to fit: R_X86_64_PC32 against undefined symbol `__imp___gmpz_set_ui'
[pplpy-0.8.3]     build/temp.cygwin-2.9.0-x86_64-2.7/ppl/bit_arrays.o: In function `Parma_Polyhedra_Library::Bit_Row::~Bit_Row()':
[pplpy-0.8.3]     /home/embray/src/sagemath/sage/local/include/ppl.hh:40618: undefined reference to `__imp___gmpz_clear'
[pplpy-0.8.3]     /home/embray/src/sagemath/sage/local/include/ppl.hh:40618:(.text+0x235): relocation truncated to fit: R_X86_64_PC32 against undefined symbol `__imp___gmpz_clear'
[pplpy-0.8.3]     build/temp.cygwin-2.9.0-x86_64-2.7/ppl/bit_arrays.o: In function `__pyx_pf_3ppl_10bit_arrays_7Bit_Row___cinit__':
[pplpy-0.8.3]     /home/embray/src/sagemath/sage/local/include/ppl.hh:40588: undefined reference to `__imp___gmpz_init'
[pplpy-0.8.3]     /home/embray/src/sagemath/sage/local/include/ppl.hh:40588:(.text+0x2c9): relocation truncated to fit: R_X86_64_PC32 against undefined symbol `__imp___gmpz_init'
[pplpy-0.8.3]     build/temp.cygwin-2.9.0-x86_64-2.7/ppl/bit_arrays.o: In function `Parma_Polyhedra_Library::Bit_Row::clear_from(unsigned long)':
[pplpy-0.8.3]     /home/embray/src/sagemath/sage/local/include/ppl.hh:40639: undefined reference to `__imp___gmpz_tdiv_r_2exp'
[pplpy-0.8.3]     /home/embray/src/sagemath/sage/local/include/ppl.hh:40639:(.text+0x1dd0): relocation truncated to fit: R_X86_64_PC32 against undefined symbol `__imp___gmpz_tdiv_r_2exp'
[pplpy-0.8.3]     build/temp.cygwin-2.9.0-x86_64-2.7/ppl/bit_arrays.o: In function `Parma_Polyhedra_Library::Bit_Row::set(unsigned long)':
[pplpy-0.8.3]     /home/embray/src/sagemath/sage/local/include/ppl.hh:40629: undefined reference to `__imp___gmpz_setbit'
[pplpy-0.8.3]     /home/embray/src/sagemath/sage/local/include/ppl.hh:40629:(.text+0x239d): relocation truncated to fit: R_X86_64_PC32 against undefined symbol `__imp___gmpz_setbit'
[pplpy-0.8.3]     collect2: error: ld returned 1 exit status
[pplpy-0.8.3]     error: command 'g++' failed with exit status 1
[pplpy-0.8.3]     Running setup.py install for pplpy: finished with status 'error'

This looks...unsurprising to me. I think I can fix this easily-enough, I hope.

comment:194 in reply to: ↑ 192 Changed 8 months ago by embray

Replying to vklein:

Ok, when exactly are spkg-postrm scripts called ? sage -f pkgname after the uninstall ?

Basically, at the end of sage-spkg-uninstall (which is called by sage -f among other things). This is also called when upgrading a package so that's why I thought it might matter eventually, at least in a minor way.

comment:195 follow-up: Changed 8 months ago by embray

  • Report Upstream changed from N/A to Reported upstream. No feedback yet.
  • Status changed from needs_review to needs_work

Upstream PR: https://gitlab.com/videlec/pplpy/merge_requests/6

We can either see if Vincent wants to make an 0.8.4 release for this fix, or I'm just as happy for now to include this as a patch in the Sage SPKG.

comment:196 in reply to: ↑ 195 Changed 8 months ago by vdelecroix

  • Report Upstream changed from Reported upstream. No feedback yet. to Reported upstream. Developers acknowledge bug.

Replying to embray:

Upstream PR: https://gitlab.com/videlec/pplpy/merge_requests/6

We can either see if Vincent wants to make an 0.8.4 release for this fix, or I'm just as happy for now to include this as a patch in the Sage SPKG.

Let us do that.

comment:197 Changed 8 months ago by vdelecroix

(the release 0.8.4)

comment:198 Changed 8 months ago by vdelecroix

  • Description modified (diff)

new release (ticket description updated)

comment:199 Changed 8 months ago by embray

That was quick, thank you :)

Not sure yet if it's a showstopper, but I'm also having a weird problem with error handling in the gmpy2 in Sage. This following example should raise a TypeError exception:

$ python -c 'import gmpy2;  gmpy2.is_fermat_prp(1234, "a")'
Aborted (core dumped)

I also built the latest gmpy2 from source, and when I run it in that source tree I get the correct behavior:

$ python -c 'import gmpy2;  gmpy2.is_fermat_prp(1234, "a")'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
TypeError: is_fermat_prp() requires 2 integer arguments

Perhaps it was just a bug in gmpy2 that has since been fixed upstream? Not sure.

comment:200 Changed 8 months ago by embray

Indeed, that particular bug was already fixed upstream: https://github.com/aleaxit/gmpy/commit/533a62cf2b2cf3673e1d6b0f3c2d0c64548a3908

I just wanted to make sure. I think I only hit on it because I was running the tests from gmpy2's current' master branch against our slightly older version, and hit that bug specifically because they added a regression test when they fixed it. Other than that all spkg-check tests pass on Cygwin for gmpy2 and pplpy :)

comment:201 Changed 8 months ago by embray

All good on my end in terms of Sage doctests, I think. I'm doing one more full run just to be sure but I can think of no reason it should work differently.

So just update to the just-released pplpy 0.8.4 and you can set back to postive_review. The spkg-postrm would be good to have too but I don't consider it a blocker.

comment:202 Changed 8 months ago by vklein

Ok i don't think doing a gmpy2 patch for this commit is worth it.

@Erik: Tell me when you're ok and i will update this ticket with pplpy's upgrade and spkg-postrm.

comment:203 Changed 8 months ago by embray

  • Owner changed from embray to vklein
  • Reviewers changed from Dima Pasechnik, François Bissey, ​Vincent Delecroix, Jeroen Demeyer to Dima Pasechnik, François Bissey, ​Vincent Delecroix, Jeroen Demeyer, Erik Bray

Yep. Once you're done with that feel free to set back to positive_review.

comment:204 Changed 8 months ago by vklein

Ok thank you for your help.

comment:205 Changed 8 months ago by git

  • Commit changed from b25b7199b237e4e732fc805ce56e40cf477f7e6d to 829317e5e5dd748e140b849ac0d5d113268278e2

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

76cfa5fTrac #23024: upgrade pplpy to 0.8.4
829317eTrac #23024: add a spkg-postrm script to clean...

comment:206 Changed 8 months ago by vklein

Upgrade to pplpy 0.8.4 and add a spkg-postrm script to clean the documentation. A reviewer should validate ​829317e in order to set this ticket in positive review.

comment:207 Changed 8 months ago by vklein

  • Owner changed from vklein to (none)

comment:208 Changed 8 months ago by vklein

  • Status changed from needs_work to needs_review

comment:209 Changed 8 months ago by vdelecroix

doing some testing...

comment:210 Changed 8 months ago by vdelecroix

  • Status changed from needs_review to positive_review

spkg-postrm works as expected. Thanks Vincent!

comment:211 Changed 8 months ago by slelievre

  • Cc slelievre added
  • Description modified (diff)
  • Keywords upgrade added

Adding "upgrade" to keywords to emphasize the need to upload new tarball to our download mirrors.

comment:212 Changed 8 months ago by vbraun

  • Branch changed from u/vklein/replacement_of_sage_libs_ppl_pyx_by_pplpy to 829317e5e5dd748e140b849ac0d5d113268278e2
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:213 follow-up: Changed 8 months ago by novoselt

  • Commit 829317e5e5dd748e140b849ac0d5d113268278e2 deleted

There were several instances of removal similar to self._PPL_C_Polyhedron.set_immutable() - is this functionality no longer supported? This was done for objects that are cached and if a user ever modifies the polyhedron, things will get wrong.

comment:214 in reply to: ↑ 213 Changed 8 months ago by vdelecroix

Replying to novoselt:

There were several instances of removal similar to self._PPL_C_Polyhedron.set_immutable() - is this functionality no longer supported? This was done for objects that are cached and if a user ever modifies the polyhedron, things will get wrong.

Indeed, it is not supported anymore. Also you can not cache a PPL_C_Polyhedron: https://gitlab.com/videlec/pplpy/blob/master/ppl/polyhedron.pyx#L86-102

In sage, the pplpy polyhedra are only used as attribute from which the user is not supposed to have direct access (unless she knows very well what she is doing). The higher level Polyhedron_base still implements this mutability flag.

Could you give a concrete example of a problematic behavior?

comment:215 Changed 8 months ago by novoselt

Well I don't have an example from "real life" given that these objects used to be immutable and that I generally knew what I am doing, but the situation I have described is not at all impossible. Although the method returning PPL representation is underscored. There is also a discrepancy with documentation at https://github.com/sagemath/sage/blob/develop/src/sage/geometry/cone.py#L1373 which claims that supplied polytope will be made immutable.

comment:216 Changed 8 months ago by vdelecroix

Please continue the discussion at #27423 since this ticket is closed.

comment:217 Changed 6 months ago by jdemeyer

  • Reviewers changed from Dima Pasechnik, François Bissey, ​Vincent Delecroix, Jeroen Demeyer, Erik Bray to Dima Pasechnik, François Bissey, Vincent Delecroix, Jeroen Demeyer, Erik Bray
Note: See TracTickets for help on using tickets.