#23024 closed enhancement (fixed)
Replacement of sage/libs/ppl.pyx by pplpy
Reported by:  vklein  Owned by:  

Priority:  major  Milestone:  sage8.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, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
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: pplpy0.8.4.tar.gz
Change History (217)
comment:1 Changed 4 years ago by
 Dependencies changed from #229 to #22928
comment:2 Changed 4 years ago by
 Type changed from PLEASE CHANGE to enhancement
comment:3 Changed 4 years ago by
 Description modified (diff)
comment:4 Changed 4 years ago by
 Description modified (diff)
comment:5 Changed 4 years ago by
 Branch set to u/vklein/replacement_of_sage_libs_ppl_pyx_by_pplpy
comment:6 Changed 4 years ago by
 Commit set to f8bf274d0a2477d621f7b8e88b79f2a23196d235
comment:7 Changed 4 years ago by
For testing purpose you can use the following tarball of pplpy (outdated, use the one in the ticket's description)
comment:8 Changed 4 years ago by
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 followup: ↓ 11 Changed 4 years ago by
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(xy)) >>> 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
comment:10 Changed 4 years ago by
 Commit changed from f8bf274d0a2477d621f7b8e88b79f2a23196d235 to dffa134588a520928393ec430bee91d0c2304fa2
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
f19f81e  Integer and rational constructors support gmpy2 params

cd8ef5d  remove OptionalExtension sage.libs.gmpy2

8ced5d3  remove get_gmpy2_path()

674fe62  Add few doctests

2ed4f95  Use of MPZ_Check and MPQ_Check functions from gmpy2 CAPI

b4f0c26  Add pplpy as a standard package

257c330  Replace sage.libs.ppl with pplpy package

636051f  Remove sage.libs.ppl extension

f52b3d4  Modify associahedron doctest

dffa134  Updating checksum for pplpy pkg

comment:11 in reply to: ↑ 9 ; followup: ↓ 13 Changed 4 years ago by
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 4 years ago by
 Commit changed from dffa134588a520928393ec430bee91d0c2304fa2 to b3563eb6a123d6aef327453061ea141e4fd1317c
Branch pushed to git repo; I updated commit sha1. New commits:
b3563eb  remove mpz_to_sage_integer function

comment:13 in reply to: ↑ 11 ; followup: ↓ 14 Changed 4 years ago by
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?
comment:14 in reply to: ↑ 13 Changed 4 years ago by
Replying to vdelecroix:
Where? The first involves a direct call
Rational(mpq_type)
and the second comparison betweenmpz
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 4 years ago by
 Commit changed from b3563eb6a123d6aef327453061ea141e4fd1317c to d3875e7b6a4f8c87c24896259cc83a922c5c989a
comment:16 Changed 4 years ago by
As pplpy has evolved to fix pickling with cPickle download pplpy again to update your local package.
comment:17 Changed 4 years ago by
 Commit changed from d3875e7b6a4f8c87c24896259cc83a922c5c989a to 7ae21f5900110c74fdb8d925558d9f30282b9630
comment:18 Changed 4 years ago by
 Commit changed from 7ae21f5900110c74fdb8d925558d9f30282b9630 to 858c2e5c6cdd8c3c85655d400c2cd535fc6ae855
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
08c7a72  Add path of gmpy2 in include_dirs

359a0ff  update checksum and package version

0d4fd2e  Integer and rational constructors support gmpy2 params

7c66a7c  remove OptionalExtension sage.libs.gmpy2

d143dde  remove get_gmpy2_path()

2c56a3f  Add few doctests

e5264fd  Use of MPZ_Check and MPQ_Check functions from gmpy2 CAPI

504ab68  Change way of creating a Rational from a mpz

072a15c  add __mpz__() to Rational

858c2e5  trac 23024 update pplpy checksum

comment:19 Changed 4 years ago by
As pplpy has evolved to fix pickling with cPickle download pplpy again to update your local package.
comment:20 Changed 3 years ago by
 Dependencies changed from #22928 to #22928, #23309
comment:21 Changed 3 years ago by
 Commit changed from 858c2e5c6cdd8c3c85655d400c2cd535fc6ae855 to 7a5812a16a76a78755784292ec2476dac990140d
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
1c1b971  Replace sage.libs.ppl with pplpy package

2a1798e  Remove sage.libs.ppl extension

d1b0b2f  Modify associahedron doctest

db21e1d  Updating checksum for pplpy pkg

eb215c3  remove mpz_to_sage_integer function

53f30f3  assert simplification ppl_lattice_polytope.py

f9a6415  remove skip=('_test_pickling') option for pplpy polyhedron

924984c  remove sage/libs/ppl from doctree

ce863ff  update checksum, snapshot version update in setup.py

7a5812a  Add path of gmpy2 in include_dirs

comment:22 Changed 3 years ago by
 Dependencies changed from #22928, #23309 to #22928
 Description modified (diff)
comment:23 Changed 3 years ago by
pplpy 0.7 is out
comment:24 Changed 3 years ago by
 Milestone changed from sage8.0 to sage8.2
comment:25 Changed 3 years ago by
 Commit changed from 7a5812a16a76a78755784292ec2476dac990140d to dc37f382de20f4ff5cba86cdd86eaf5100af8f22
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
760bbdf  Modify associahedron doctest

6d3345e  Updating checksum for pplpy pkg

85cb9fc  remove mpz_to_sage_integer function

70177c8  assert simplification ppl_lattice_polytope.py

b0858a2  remove skip=('_test_pickling') option for pplpy polyhedron

94c7817  remove sage/libs/ppl from doctree

67c8c2e  update checksum, snapshot version update in setup.py

50d62ff  Add path of gmpy2 in include_dirs

375fda6  Trac #23024 Fix checksum (pplpy), spkginstall and spkgcheck

dc37f38  Trac #23024 Add new mpz gmpy2 to sage Integer conversion

comment:26 Changed 3 years ago by
Rebase on 22928 (with 22928 based on 8.2.beta0)
comment:27 Changed 3 years ago by
You need to provide a link to the pplpy tarball.
comment:28 Changed 3 years ago by
 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 3 years ago by
 Description modified (diff)
comment:30 followup: ↓ 31 Changed 3 years ago by
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 3 years ago by
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 3 years ago by
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.
comment:33 Changed 3 years ago by
 Cc fbissey added
comment:34 Changed 3 years ago by
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 3 years ago by
 Commit changed from dc37f382de20f4ff5cba86cdd86eaf5100af8f22 to 7d191546377db00ef950b8936895f58b48b25b85
Branch pushed to git repo; I updated commit sha1. New commits:
7d19154  Trac #23024 SPKG.txt add upstream contact and licence.

comment:36 Changed 3 years ago by
Done, thanks for noticing that.
comment:37 Changed 3 years ago by
 Commit changed from 7d191546377db00ef950b8936895f58b48b25b85 to 01403bd74b0863cc11422a13f95051a84cb9a291
Branch pushed to git repo; I updated commit sha1. New commits:
01403bd  Trac #23024 Deprecate sage.libs.ppl

comment:38 followup: ↓ 39 Changed 3 years ago by
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 3 years ago by
comment:40 Changed 3 years ago by
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/sitepackages/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/sitepackages/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/sitepackages/sage/geometry/lattice_polytope.py", line 3680, in points l = gcd(v) NameError: global name 'gcd' is not defined
comment:41 Changed 3 years ago by
 Commit changed from 01403bd74b0863cc11422a13f95051a84cb9a291 to d244109bd7ed07fa1c3cbec2a08f68a2e2730a74
comment:42 Changed 3 years ago by
Yes i have see this one. The last remaining problem is a SEGFAULT when running ppl_lattice_polytope.py doctests.
comment:43 followup: ↓ 44 Changed 3 years ago by
Could you post the log here?
comment:44 in reply to: ↑ 43 Changed 3 years ago by
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.
comment:45 followup: ↓ 48 Changed 3 years ago by
comment:46 Changed 3 years ago by
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 3 years ago by
See #24417
comment:48 in reply to: ↑ 45 Changed 3 years ago by
comment:49 Changed 3 years ago by
 Commit changed from d244109bd7ed07fa1c3cbec2a08f68a2e2730a74 to 3cec63259adb968dc3b70d4514d21c3e5530842f
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
7596cff  remove sage/libs/ppl from doctree

0cd326c  update checksum, snapshot version update in setup.py

91b5d52  Add path of gmpy2 in include_dirs

0b965a7  Trac #23024 Fix checksum (pplpy), spkginstall and spkgcheck

dd886fb  Trac #23024 Add new mpz gmpy2 to sage Integer conversion

b7a5a21  Trac #23024 SPKG.txt add upstream contact and licence.

96146ca  Trac #23024 Deprecate sage.libs.ppl

e85ae5f  Trac #23024 Use gmpy2 as a standard package

da9aa35  Trac #23024 Restore import gcd

3cec632  Trac #23024 : merge #22928 ppl.pyx add deprecation warning ...

comment:50 Changed 3 years ago by
 Commit changed from 3cec63259adb968dc3b70d4514d21c3e5530842f to d206c1432ef3bd7f394921de43363fb4bdc84188
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
d206c14  Trac #23024: ppl.pyx add deprecation warning to long doctest

comment:51 Changed 3 years ago by
You are adding sage.libs.gmpy2
in 7596cff
and removing it in 91b5d52
...
comment:52 Changed 3 years ago by
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 3 years ago by
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 3 years ago by
There are a lot of trailing whitespaces that should be removed.
comment:55 Changed 3 years ago by
 Commit changed from d206c1432ef3bd7f394921de43363fb4bdc84188 to f3e4bcd318536643b1b8a4bf6e3560813e0970bd
comment:56 Changed 3 years ago by
History rewrited. Trailing whitespaces removed in real_mpfr.pyx
, complex_mpc.pyx
and complex_number.pyx
files.
comment:57 Changed 3 years ago by
I don't understand why you are modifying pplpy/SPKG.txt
, pplpy/checksums.ini
, pplpy/spkgcheck
, etc in both 886424b
and a6e1cc7
.
comment:58 Changed 3 years ago by
Build fine and all tests pass on my computer!
comment:59 Changed 3 years ago by
 Description modified (diff)
comment:60 Changed 3 years ago by
Use sdh_pip_install
instead of $PIP_INSTALL
comment:61 followup: ↓ 63 Changed 3 years ago by
Is the license of pplpy really GPL version 3
? Or do you mean GPL version 3 or later
? The difference is important.
comment:62 followup: ↓ 66 Changed 3 years ago by
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 ; followup: ↓ 65 Changed 3 years ago by
Replying to jdemeyer:
Is the license of pplpy really
GPL version 3
? Or do you meanGPL 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 3 years ago by
In backend_ppl.py
you do from gmpy2 import mpz
but you're not using it.
comment:65 in reply to: ↑ 63 Changed 3 years ago by
Replying to vklein:
Is it better to say
GPL version 3 or later
anyway ?
comment:66 in reply to: ↑ 62 ; followup: ↓ 68 Changed 3 years ago by
comment:67 Changed 3 years ago by
 Commit changed from f3e4bcd318536643b1b8a4bf6e3560813e0970bd to ef97c325b862e31856dc63c127c979692d7d31f5
Branch pushed to git repo; I updated commit sha1. New commits:
ef97c32  Trac #23024: spkginstall `sdh_pip_install` instead ...

comment:68 in reply to: ↑ 66 ; followup: ↓ 69 Changed 3 years ago by
comment:69 in reply to: ↑ 68 Changed 3 years ago by
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.
comment:70 Changed 3 years ago by
 Commit changed from ef97c325b862e31856dc63c127c979692d7d31f5 to 2f98d707835ee148592d341666babec1d79e1419
Branch pushed to git repo; I updated commit sha1. New commits:
2f98d70  Trac #23024: Code cleaning

comment:71 Changed 3 years ago by
selftesting fails:
$ SAGE_CHECK=yes ./sage f pplpy ... [pplpy0.7] Successfully installed pplpy0.7 [pplpy0.7] Cleaning up... [pplpy0.7] [pplpy0.7] real 0m38.345s [pplpy0.7] user 0m36.490s [pplpy0.7] sys 0m1.835s [pplpy0.7] Copying package files from temporary location /home/dima/Sage/sagedev/local/var/tmp/sage/build/pplpy0.7/inst to /home/dima/Sage/sagedev/local [pplpy0.7] Running the test suite for pplpy0.7... [pplpy0.7] running test [pplpy0.7] error: cannot copy tree './tests': not a directory [pplpy0.7]
comment:72 Changed 3 years ago by
comment:73 Changed 3 years ago by
 Cc embray added
comment:74 Changed 2 years ago by
 Dependencies #22928 deleted
comment:75 Changed 2 years ago by
 Milestone changed from sage8.2 to sagepending
This would still be a good thing to do IMO, in general.
comment:76 Changed 2 years ago by
 Description modified (diff)
comment:77 Changed 2 years ago by
 Description modified (diff)
 Milestone changed from sagepending to sage8.7
comment:78 Changed 2 years ago by
 Commit changed from 2f98d707835ee148592d341666babec1d79e1419 to 55ad161c47d0a901cf9e81d2adeb09b94c66d655
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
f5b7110  Trac 23024: Add pplpy as a standard package

82a5d24  Trac 23024: Replace sage.libs.ppl with pplpy package

59617e1  Trac #23024: Use gmpy2 as a standard package

8f51887  Trac #23024: spkginstall `sdh_pip_install` instead ...

d7a67d4  Trac #23024: Code cleaning

9ea9382  Trac #23024: Remove HAVE_GMPY2 from coerce.pyx

55ad161  Trac #23024: fix ppl_lattice_polytope doctests

comment:79 Changed 2 years ago by
Rebased on sage8.6
comment:80 Changed 2 years ago by
 Description modified (diff)
comment:81 Changed 2 years ago by
 Commit changed from 55ad161c47d0a901cf9e81d2adeb09b94c66d655 to e90bc7781e0685485908bb781fb6ea78eb28ac10
comment:83 Changed 2 years ago by
Apparently a merge failure with sage 8.7.beta0
comment:84 Changed 2 years ago by
 Commit changed from e90bc7781e0685485908bb781fb6ea78eb28ac10 to 9a065413932ab5d11d86789ddf687b471f88f699
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
c37dcb0  Trac 23024: Add pplpy as a standard package

3f5eed8  Trac 23024: Replace sage.libs.ppl with pplpy package

26d3d54  Trac #23024: Use gmpy2 as a standard package

1a5d7e5  Trac #23024: spkginstall `sdh_pip_install` instead ...

62452d5  Trac #23024: Code cleaning

58a0e8a  Trac #23024: Remove HAVE_GMPY2 from coerce.pyx

dabd865  Trac #23024: fix ppl_lattice_polytope doctests

9a06541  Trac #23024: upgrade pplpy version to 0.8.1

comment:85 Changed 2 years ago by
Solve merge failure with sage 8.7.beta0.
comment:86 Changed 2 years ago by
 offtopic  Vincent, please provide info on
comment:87 Changed 2 years ago by
 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 2 years ago by
Why
 sage: TestSuite(P).run(skip='_test_pickling') + sage: TestSuite(P).run(skip=('_test_pickling'))
comment:89 followup: ↓ 94 Changed 2 years ago by
And
 def _repr_(self): + def __repr__(self):
comment:90 Changed 2 years ago by
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 followup: ↓ 92 Changed 2 years ago by
Does pplpy build/install its own docs? This kind of setup is used e.g. in cvxopt.
comment:92 in reply to: ↑ 91 Changed 2 years ago by
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 followup: ↓ 95 Changed 2 years ago by
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 2 years ago by
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 ; followups: ↓ 98 ↓ 100 Changed 2 years ago by
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.
comment:96 Changed 2 years ago by
 Commit changed from 9a065413932ab5d11d86789ddf687b471f88f699 to 13507e4fc42743661147a4071ef7be95a3871c45
Branch pushed to git repo; I updated commit sha1. New commits:
13507e4  Trac #23024: Implement reviewer's comments : add depende...

comment:97 Changed 2 years ago by
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 2 years ago by
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 notyes
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:
 remove all links in the Sage documentation
 in each relevant module, make a reference to the pplpy development website (https://gitlab.com/videlec/pplpy) and pplpy documentation website (http://www.labri.fr/perso/vdelecro/pplpy/latest/).
comment:99 Changed 2 years ago by
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 ; followup: ↓ 101 Changed 2 years ago by
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 notyes
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 ; followup: ↓ 103 Changed 2 years ago by
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 packagespecific 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 2 years ago by
 Commit changed from 13507e4fc42743661147a4071ef7be95a3871c45 to 665419ad8dc012687220cb40ad6c09a943ca9b44
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
665419a  Trac #23024: Implement reviewer's comments ...

comment:103 in reply to: ↑ 101 Changed 2 years ago by
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 packagespecific 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 2 years ago by
 Commit changed from 665419ad8dc012687220cb40ad6c09a943ca9b44 to ef8997834f73c34d983260c0ba782041d0966a02
Branch pushed to git repo; I updated commit sha1. New commits:
ef89978  Trac #23024: Generate and installl doc

comment:105 Changed 2 years ago by
For testing purpose use https://www.labri.fr/perso/vklein/pplpy0.8.2b0.tar.gz with ef89978
.
comment:106 Changed 2 years ago by
 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/sitepackages
.
I haven't find an environment variable for lib/python2.7/sitepackages
or something equivalent.
Any help is welcomed.
comment:107 Changed 2 years ago by
 Commit changed from ef8997834f73c34d983260c0ba782041d0966a02 to d8c41f665958133b323380f70800a66c6432820a
Branch pushed to git repo; I updated commit sha1. New commits:
d8c41f6  Trac #23024: Add intersphinx between sage doc ...

comment:108 Changed 2 years ago by
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 followup: ↓ 110 Changed 2 years ago by
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 2 years ago by
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 followup: ↓ 119 Changed 2 years ago by
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 followup: ↓ 113 Changed 2 years ago by
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 ; followup: ↓ 114 Changed 2 years ago by
Replying to vdelecroix:
Crossed links does not work for me (e.g. the
ppl.C_Polyhedron
in the fileen/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 docclean
. `ppl.C_Polyhedron`
should have been replaced by `ppl.polyhedron.C_Polyhedron`
.
comment:114 in reply to: ↑ 113 Changed 2 years ago by
Replying to vklein:
Replying to vdelecroix:
Crossed links does not work for me (e.g. the
ppl.C_Polyhedron
in the fileen/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 docclean
.`ppl.C_Polyhedron`
should have been replaced by`ppl.polyhedron.C_Polyhedron`
.
I did make docclean
, sage f pplpy
and make doc
.
comment:115 Changed 2 years ago by
I know what I did wrong: I did not pull your last commits... sorry for the noise.
comment:116 Changed 2 years ago by
It is working now!
comment:117 Changed 2 years ago by
I will make a new release of pplpy.
Could you ask about 106 on sagedevel? 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:118 Changed 2 years ago by
For comment:106 : link to the post on sagedevel https://groups.google.com/forum/#!topic/sagedevel/IOn7wneT8xo
comment:119 in reply to: ↑ 111 Changed 2 years ago by
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 followups: ↓ 121 ↓ 122 Changed 2 years ago by
Alternative: include the compiled doc in the source tarball so that the spkginstall script just need to copy it...
comment:121 in reply to: ↑ 120 ; followup: ↓ 123 Changed 2 years ago by
Replying to vdelecroix:
Alternative: include the compiled doc in the source tarball so that the spkginstall script just need to copy it...
1. I'd rather run the installation twice than this.
comment:122 in reply to: ↑ 120 ; followup: ↓ 124 Changed 2 years ago by
Replying to vdelecroix:
Alternative: include the compiled doc in the source tarball so that the spkginstall 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 2 years ago by
Replying to dimpase:
Replying to vdelecroix:
Alternative: include the compiled doc in the source tarball so that the spkginstall 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).
comment:124 in reply to: ↑ 122 Changed 2 years ago by
Replying to vklein:
Replying to vdelecroix:
Alternative: include the compiled doc in the source tarball so that the spkginstall 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 deinstallation. Besides, there is no guarantee it would look nice enough, so that it matches the current Sage docs style, etc.
comment:125 Changed 2 years ago by
Ok i see. And i agree that having the same sphinx install generating both documentation is a good thing.
comment:126 Changed 2 years ago by
 Commit changed from d8c41f665958133b323380f70800a66c6432820a to c40f656f7f605435339e33a3ed1d4ac8598852fb
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
41e16e6  Trac #23024: Use gmpy2 as a standard package

68e0e45  Trac #23024: spkginstall `sdh_pip_install` instead ...

9da630f  Trac #23024: Code cleaning

273b772  Trac #23024: Remove HAVE_GMPY2 from coerce.pyx

1365deb  Trac #23024: fix ppl_lattice_polytope doctests

3da5acf  Trac #23024: upgrade pplpy version to 0.8.1

9fed2a3  Trac #23024: Implement reviewer's comments ...

33f4759  Trac #23024: Generate and installl doc

3cf578c  Trac #23024: Add intersphinx between sage doc ...

c40f656  Trac #23024: Upgrade to pplpy version 8.2

comment:127 Changed 2 years ago by
 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 2 years ago by
 Commit changed from c40f656f7f605435339e33a3ed1d4ac8598852fb to 93e0b66d6d76cdb867ec049a7be12c123a2ba769
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
bcb1373  Trac #23024: Add pplpy as a standard package

1aec143  Trac #23024: Replace sage.libs.ppl with pplpy package

3231e47  Trac #23024: Use gmpy2 as a standard package

93e0b66  Trac #23024: Doc and dependencies: ...

comment:129 Changed 2 years ago by
 Status changed from needs_work to needs_review
Rewrite ticket history.
comment:130 followup: ↓ 133 Changed 2 years ago by
To my mind, a better solution has to be found for
+ # Compile pplpy's documentation + sagepython23 setup.py build_ext inplace + PYTHONPATH=$PYTHONPATH:$(pwd) + cd docs + $MAKE html
comment:131 Changed 2 years ago by
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?
comment:132 Changed 2 years ago by
 Status changed from needs_review to needs_work
Yes, this is the discussion link https://groups.google.com/forum/?#!topic/sagedevel/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 2 years ago by
Replying to vdelecroix:
To my mind, a better solution has to be found for
+ # Compile pplpy's documentation + sagepython23 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 2 years ago by
I haven't find a decisive difference between the two Makefile. The most obvious one being that pplpy use sphinxbuild b
and sagenb use sphinxbuild 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.
comment:135 Changed 2 years ago by
 Commit changed from 93e0b66d6d76cdb867ec049a7be12c123a2ba769 to 8d020eac9061705ebe900a4be3ec1b3c66d1eb49
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
e2d3e07  Trac #23024: Add pplpy as a standard package

ba656ae  Trac #23024: Replace sage.libs.ppl with pplpy package

5ac4d45  Trac #23024: Use gmpy2 as a standard package

c4a7255  Trac #23024: Doc and dependencies: ...

8d020ea  Trac #23024: Sphinx doc avoid to compile two ...

comment:136 Changed 2 years ago by
Add a workaround to avoid two build and rebase on 8.7.beta3.
comment:137 Changed 2 years ago by
It is not working for me
[pplpy0.8.2] WARNING: autodoc: failed to import module u'ppl'; the following exception was raised: [pplpy0.8.2] No module named ppl [pplpy0.8.2] WARNING: autodoc: failed to import module u'ppl.constraint'; the following exception was raised: [pplpy0.8.2] No module named ppl.constraint [pplpy0.8.2] WARNING: autodoc: failed to import module u'ppl.linear_algebra'; the following exception was raised: [pplpy0.8.2] No module named ppl.linear_algebra [pplpy0.8.2] WARNING: autodoc: failed to import module u'ppl.generator'; the following exception was raised: [pplpy0.8.2] No module named ppl.generator [pplpy0.8.2] WARNING: autodoc: failed to import module u'ppl.polyhedron'; the following exception was raised: [pplpy0.8.2] No module named ppl.polyhedron [pplpy0.8.2] WARNING: autodoc: failed to import module u'ppl.mip_problem'; the following exception was raised: [pplpy0.8.2] No module named ppl.mip_problem
comment:138 followup: ↓ 144 Changed 2 years ago by
And shouldn't python
be python23
?
comment:139 Changed 2 years ago by
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/sitepackages
And using string replace
is not an option
>>> import site >>> site.getusersitepackages() '/home/vincent/.sage/local/lib/python2.7/sitepackages' >>> site.getuserbase() '/home/vincent/.sage//local'
comment:140 Changed 2 years ago by
This already looks more reasonable
>>> import sysconfig >>> sysconfig.get_paths(expand=False) {'purelib': '{base}/lib/python{py_version_short}/sitepackages', '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}/sitepackages' }
comment:141 Changed 2 years ago by
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/sitepackages'
comment:142 Changed 2 years ago by
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]}\\sitepackages' if sys.platform == 'darwin' and sys._framework: return f'{userbase}/lib/python/sitepackages' return f'{userbase}/lib/python{version[0]}.{version[1]}/sitepackages'
Python version is not always used
comment:143 Changed 2 years ago by
 Commit changed from 8d020eac9061705ebe900a4be3ec1b3c66d1eb49 to 4b462ab0e597aba8d08286b5fdfd97855b9f8e8a
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
4b462ab  Trac #23024: Sphinx doc avoid to compile two ...

comment:144 in reply to: ↑ 138 Changed 2 years ago by
Replying to vdelecroix:
And shouldn't
python
bepython23
?
Yes you are right.
Workaround fixed and enhanced with Erik Bray's help.
comment:145 Changed 2 years ago by
Same as before
[pplpy0.8.2] WARNING: autodoc: failed to import module u'ppl'; the following exception was raised: [pplpy0.8.2] No module named ppl [pplpy0.8.2] WARNING: autodoc: failed to import module u'ppl.constraint'; the following exception was raised: [pplpy0.8.2] No module named ppl.constraint [pplpy0.8.2] WARNING: autodoc: failed to import module u'ppl.linear_algebra'; the following exception was raised: [pplpy0.8.2] No module named ppl.linear_algebra [pplpy0.8.2] WARNING: autodoc: failed to import module u'ppl.generator'; the following exception was raised: [pplpy0.8.2] No module named ppl.generator [pplpy0.8.2] WARNING: autodoc: failed to import module u'ppl.polyhedron'; the following exception was raised: [pplpy0.8.2] No module named ppl.polyhedron [pplpy0.8.2] WARNING: autodoc: failed to import module u'ppl.mip_problem'; the following exception was raised: [pplpy0.8.2] No module named ppl.mip_problem
comment:146 Changed 2 years ago by
Can you post or mail me the whole log ?
comment:147 Changed 2 years ago by
Ok i have an obvious bug in my spgk_install
file
comment:148 Changed 2 years ago by
 Commit changed from 4b462ab0e597aba8d08286b5fdfd97855b9f8e8a to f4fe72be6c56056edf06850640459b2b1986bcea
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
f4fe72b  Trac #23024: Sphinx doc avoid to compile two ...

comment:149 Changed 2 years ago by
It's fixed sorry for the previous commit.
comment:150 Changed 2 years ago by
Why does sphinx inventory connect to internet!?
[pplpy0.8.2] loading intersphinx inventory from http://docs.python.org/objects.inv...
comment:151 Changed 2 years ago by
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)
comment:153 Changed 2 years ago by
 Commit changed from f4fe72be6c56056edf06850640459b2b1986bcea to abaef5ef92cf1d2148602527c350957c76ef7e14
Branch pushed to git repo; I updated commit sha1. New commits:
abaef5e  Trac #23024: upgrade pplpy to version 0.8.3

comment:154 followup: ↓ 155 Changed 2 years ago by
 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/sagedevel/qoxVng1__0A first.
comment:155 in reply to: ↑ 154 Changed 2 years ago by
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/sagedevel/qoxVng1__0A first.
What is the status of the discussion now?
comment:156 followup: ↓ 157 Changed 2 years ago by
Note: for the documentation build, there is also the possibility to use the spkgpostinst
script (that is run after the installation in $SAGE_LOCAL
).
comment:157 in reply to: ↑ 156 ; followup: ↓ 158 Changed 2 years ago by
Replying to vdelecroix:
Note: for the documentation build, there is also the possibility to use the
spkgpostinst
script (that is run after the installation in$SAGE_LOCAL
).
Yes that's true. The reason to keep the documentation build in spkginst
is because it's currently the standard way of doing it. Tell me what you prefer.
comment:158 in reply to: ↑ 157 Changed 2 years ago by
Replying to vklein:
Replying to vdelecroix:
Note: for the documentation build, there is also the possibility to use the
spkgpostinst
script (that is run after the installation in$SAGE_LOCAL
).Yes that's true. The reason to keep the documentation build in
spkginst
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 2 years ago by
 Commit changed from abaef5ef92cf1d2148602527c350957c76ef7e14 to c6d75a9b610220d7e882a2be9f83591f81fbc480
Branch pushed to git repo; I updated commit sha1. New commits:
c6d75a9  Trac #23024: Add a spkgpostinst script to man...

comment:160 Changed 2 years ago by
Ok seems fair. spkgpostinst
script added.
comment:161 Changed 2 years ago by
Having a comment in the spkginstall
script is good but I think that it would be even nicer with an explanation of why this is moved inside the spkgpostinst
script.
comment:162 Changed 2 years ago by
 Commit changed from c6d75a9b610220d7e882a2be9f83591f81fbc480 to 794aed8c6a61b9397167c7cfe8c2925ae4e90370
Branch pushed to git repo; I updated commit sha1. New commits:
794aed8  Trac #23024: spkginstall: Document the reason ...

comment:163 Changed 2 years ago by
Ok, explanation added.
comment:164 Changed 2 years ago by
The current status is good for me. Thanks Vincent.
Any comment Dima?
comment:165 Changed 2 years ago by
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 2 years ago by
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.
comment:167 Changed 2 years ago by
@Dima tell me if you think it's ok or if you think ppl_backend.pyx
should be reworked in another way.
comment:168 followup: ↓ 169 Changed 2 years ago by
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 ; followup: ↓ 171 Changed 2 years ago by
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 2 years ago by
 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 2 years ago by
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_CoefficientThis 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 (fromchar
tolong 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 2 years ago by
 Reviewers changed from Dima Pasechnik to Dima Pasechnik, François Bissey, Vincent Delecroix, Jeroen Demeyer
Thanks to all reviewers.
comment:174 Changed 2 years ago by
 Commit changed from 794aed8c6a61b9397167c7cfe8c2925ae4e90370 to 81bf994a34c7f862cfb71d3a76ab4632017607ae
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
477a7be  Trac #23024: Add pplpy as a standard package

44b61c5  Trac #23024: Replace sage.libs.ppl with pplpy package

a4c722c  Trac #23024: Use gmpy2 as a standard package

684e107  Trac #23024: Doc and dependencies: ...

b49007c  Trac #23024: Sphinx doc avoid to compile two ...

bfc737a  Trac #23024: upgrade pplpy to version 0.8.3

b7e3547  Trac #23024: Add a spkgpostinst script to man...

81bf994  Trac #23024: spkginstall: Document the reason ...

comment:175 Changed 2 years ago by
Rebase on 8.7.beta5
comment:176 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:177 Changed 2 years ago by
 Status changed from needs_review to positive_review
comment:178 followups: ↓ 179 ↓ 180 Changed 2 years ago by
 Status changed from positive_review to needs_work
Looking at the diff, I have several minor comments:
 Don't change whitespace in
src/module_list.py
for no good reason.
 In various places, you use
list(...)
. Why not use a list comprehension instead?
 In multiline imports, the preferred style is to use parentheses instead of
\
.
 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.
 What's the meaning of the
/...
at the start of the deprecation warnings in doctests? I have never seen that.
 In
_add_rational_constraint
, why do youcdef Rational rhs
? Sincenumerator
anddenominator
are pure Python methods, this is a pessimization.
comment:179 in reply to: ↑ 178 Changed 2 years ago by
Replying to jdemeyer:
Looking at the diff, I have several minor comments:
 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 2 years ago by
 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/sageeval
.
comment:181 Changed 2 years ago by
I see now. Those aren't "normal" doctests, they are actually run in a subprocess.
comment:182 Changed 2 years ago by
 Commit changed from 81bf994a34c7f862cfb71d3a76ab4632017607ae to b25b7199b237e4e732fc805ce56e40cf477f7e6d
Branch pushed to git repo; I updated commit sha1. New commits:
b25b719  Trac #23024: Implements reviewer's comment :

comment:183 Changed 2 years ago by
 Status changed from needs_work to needs_review
Comments 1,2,3,4 and 6 implemented.
comment:184 Changed 2 years ago by
 Priority changed from minor to major
comment:185 Changed 2 years ago by
Jeroen?
comment:186 followup: ↓ 187 Changed 2 years ago by
 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 2 years ago by
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 2 years ago by
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 2 years ago by
 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 2 years ago by
 Owner changed from (none) to embray
comment:191 Changed 2 years ago by
One minor comment just looking at the patch: To complement the pplpy/spkgpostinst
script, it would be good, albeit not strictly necessary, to include a pplpy/spkgpostrm
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 followup: ↓ 194 Changed 2 years ago by
Ok, when exactly are spkgpostrm
scripts called ? sage f pkgname
after the uninstall ?
Anyways i will wait for Owner field to be empty before pushing any new commit.
comment:193 Changed 2 years ago by
Yeah, pplpy doesn't build on Cygwin:
[pplpy0.8.3] g++ shared Wl,enableautoimagebase 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.cygwin2.9.0x86_642.7/ppl/bit_arrays.o build/temp.cygwin2.9.0x86_642.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.cygwin2.9.0x86_642.7/ppl/bit_arrays.dll [pplpy0.8.3] build/temp.cygwin2.9.0x86_642.7/ppl/bit_arrays.o: In function `Parma_Polyhedra_Library::Bit_Row::clear()': [pplpy0.8.3] /home/embray/src/sagemath/sage/local/include/ppl.hh:40661: undefined reference to `__imp___gmpz_set_ui' [pplpy0.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' [pplpy0.8.3] build/temp.cygwin2.9.0x86_642.7/ppl/bit_arrays.o: In function `Parma_Polyhedra_Library::Bit_Row::~Bit_Row()': [pplpy0.8.3] /home/embray/src/sagemath/sage/local/include/ppl.hh:40618: undefined reference to `__imp___gmpz_clear' [pplpy0.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' [pplpy0.8.3] build/temp.cygwin2.9.0x86_642.7/ppl/bit_arrays.o: In function `__pyx_pf_3ppl_10bit_arrays_7Bit_Row___cinit__': [pplpy0.8.3] /home/embray/src/sagemath/sage/local/include/ppl.hh:40588: undefined reference to `__imp___gmpz_init' [pplpy0.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' [pplpy0.8.3] build/temp.cygwin2.9.0x86_642.7/ppl/bit_arrays.o: In function `Parma_Polyhedra_Library::Bit_Row::clear_from(unsigned long)': [pplpy0.8.3] /home/embray/src/sagemath/sage/local/include/ppl.hh:40639: undefined reference to `__imp___gmpz_tdiv_r_2exp' [pplpy0.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' [pplpy0.8.3] build/temp.cygwin2.9.0x86_642.7/ppl/bit_arrays.o: In function `Parma_Polyhedra_Library::Bit_Row::set(unsigned long)': [pplpy0.8.3] /home/embray/src/sagemath/sage/local/include/ppl.hh:40629: undefined reference to `__imp___gmpz_setbit' [pplpy0.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' [pplpy0.8.3] collect2: error: ld returned 1 exit status [pplpy0.8.3] error: command 'g++' failed with exit status 1 [pplpy0.8.3] Running setup.py install for pplpy: finished with status 'error'
This looks...unsurprising to me. I think I can fix this easilyenough, I hope.
comment:194 in reply to: ↑ 192 Changed 2 years ago by
Replying to vklein:
Ok, when exactly are
spkgpostrm
scripts called ?sage f pkgname
after the uninstall ?
Basically, at the end of sagespkguninstall
(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 followup: ↓ 196 Changed 2 years ago by
 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 2 years ago by
 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 2 years ago by
(the release 0.8.4)
comment:198 Changed 2 years ago by
 Description modified (diff)
new release (ticket description updated)
comment:199 Changed 2 years ago by
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 2 years ago by
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 spkgcheck
tests pass on Cygwin for gmpy2 and pplpy :)
comment:201 Changed 2 years ago by
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 justreleased pplpy 0.8.4 and you can set back to postive_review. The spkgpostrm would be good to have too but I don't consider it a blocker.
comment:202 Changed 2 years ago by
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 spkgpostrm
.
comment:203 Changed 2 years ago by
 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 2 years ago by
Ok thank you for your help.
comment:205 Changed 2 years ago by
 Commit changed from b25b7199b237e4e732fc805ce56e40cf477f7e6d to 829317e5e5dd748e140b849ac0d5d113268278e2
comment:206 Changed 2 years ago by
Upgrade to pplpy 0.8.4 and add a spkgpostrm script to clean the documentation.
A reviewer should validate 829317e
in order to set this ticket in positive review.
comment:207 Changed 2 years ago by
 Owner changed from vklein to (none)
comment:208 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:209 Changed 2 years ago by
doing some testing...
comment:210 Changed 2 years ago by
 Status changed from needs_review to positive_review
spkgpostrm
works as expected. Thanks Vincent!
comment:211 Changed 2 years ago by
 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 2 years ago by
 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 followup: ↓ 214 Changed 2 years ago by
 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 2 years ago by
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#L86102
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 2 years ago by
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 2 years ago by
Please continue the discussion at #27423 since this ticket is closed.
comment:217 Changed 23 months ago by
 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
Branch pushed to git repo; I updated commit sha1. New commits:
Updating checksum for pplpy pkg