New issue

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

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

Already on GitHub? Sign in to your account

D4.10: Second version of the PARI Python/Cython bindings #84

Closed
minrk opened this Issue Sep 8, 2015 · 48 comments

Comments

@minrk
Contributor

minrk commented Sep 8, 2015

cc: @defeo, @jdemeyer

The PARI library is a state-of-the-art library for number theory. It is an important component of the SAGE computational system: Sage uses it for example to implement number fields and elliptic curves. PARI itself is just a C library but it comes with a command-line interface called GP. Together, these form the software package PARI/GP. Thanks to OpenDreamKit, see D4.4 (#93), there is also a JUPYTER interface implementing the same language. Unfortunately, GP is a specialised language which is not so easy to integrate with other software.

This deliverable is a Python interface for PARI/GP. Since Python is a widely-used programming language, this makes it easy to integrate PARI/GP with other (scientific) software. As such, it is an important component of a VRE for researchers who require number-theoretical computations.
Since SAGE is implemented in Python, it is also necessary for SAGE to have a Python interface to PARI/GP.

In the first reporting period of the Open Dream Kit project, we created PYTHON bindings for PARI/GP in a package called cypari2. We reported on that in D4.1 (#83) and this deliverable is a follow-up.

@minrk minrk added this to the D4.10 milestone Sep 8, 2015

@nthiery nthiery referenced this issue Sep 11, 2015

Closed

First informal review #156

3 of 3 tasks complete

@minrk minrk self-assigned this Nov 3, 2015

@minrk minrk assigned videlec and unassigned minrk Dec 30, 2015

@nthiery nthiery modified the milestones: Month 36: 2018-08-31, D4.10 Mar 22, 2016

@defeo

This comment has been minimized.

Contributor

defeo commented Mar 22, 2016

What's funny is that this deliverable (NT: in fact the corresponding task) promises:

  • Use the declaration files of GP to automatise CYTHON declarations.
  • Replace copy from the PARI stack by direct pointers.
  • More tests and documentation.
  • Integrate the parallelisation features from T5.1 within PYTHON .
  • Implement a crossed bug report system between SAGE and PARI (using results of T7.3).

And, while #83 is not ready yet, items 1-2 have already been done, and 3 will certainly follow.

@jdemeyer

This comment has been minimized.

Contributor

jdemeyer commented Mar 22, 2016

items 1-2 have already been done

Item 2 has certainly not been done.

For item 1, it depends on exactly what is meant: I guess it refers to automatically generating the Cython declarations for the PARI library functions. This has not been done at all. And it's not clear what "declaration files of GP" refers to. I guess it should be "PARI header files".

@defeo

This comment has been minimized.

Contributor

defeo commented Mar 22, 2016

Hmmm. I had misinterpreted item 2. But then, is it even possible/advisable to do so?

@KBelabas

This comment has been minimized.

Contributor

KBelabas commented Mar 23, 2016

  • jdemeyer [2016-03-22 17:45]:

    items 1-2 have already been done

    Item 2 has certainly not been done.

    For item 1, it depends on exactly what is meant: I guess it refers to automatically generating the Cython declarations for the PARI library functions. This has not been done at all. And it's not clear what "declaration files of GP" refers to. I guess it should be "PARI header files".

I guess it should be the "description system for GP functions as
summarized in src/desc/pari.desc in PARI source distribution"

Cheers,

K.B.

Karim Belabas, IMB (UMR 5251) Tel: (+33) (0)5 40 00 26 17
Universite de Bordeaux Fax: (+33) (0)5 40 00 69 50
351, cours de la Liberation http://www.math.u-bordeaux.fr/~kbelabas/
F-33405 Talence (France) http://pari.math.u-bordeaux.fr/ [PARI/GP]
`

@jdemeyer

This comment has been minimized.

Contributor

jdemeyer commented Mar 23, 2016

I guess it should be the "description system for GP functions as summarized in src/desc/pari.desc in PARI source distribution"

OK, let's try again. There are two things which could potentially be auto-generated:

(1) Python functions/methods corresponding to GP functions. This is implemented in Sage, using pari.desc.
(2) Cython declarations for PARI library functions. This is not at all implemented and is probably what the deliverable refers to. To do this, you would need to use the PARI header files.

@KBelabas

This comment has been minimized.

Contributor

KBelabas commented Mar 23, 2016

  • jdemeyer [2016-03-23 09:21]:

    I guess it should be the "description system for GP functions as summarized in src/desc/pari.desc in PARI source distribution"

    OK, let's try again. There are two things which could potentially be auto-generated:

    (1) Python functions/methods corresponding to GP functions. This is implemented in Sage, using pari.desc.

Good.

(2) Cython declarations for PARI library functions. This is not at all
implemented and is probably what the deliverable refers to. To do
this, you would need to use the PARI header files.

Thanks for the clarification. Hum, paridecl.h is easy to parse if you
remove the functions taking (context + functions) as argument
[ implementing iterators, numerical summation/integration methods ], but most
useful ones are probably already catered for as GP functions. Filtering
those out involves nothing more than ignoring lines containing the '(*'
pattern.

What is the intent there ? 100% coverage ? (about 4500 functions);
95% automatic coverage ٍ+ hand-picking a few useful leftovers ?

Cheers,

K.B.

Karim Belabas, IMB (UMR 5251) Tel: (+33) (0)5 40 00 26 17
Universite de Bordeaux Fax: (+33) (0)5 40 00 69 50
351, cours de la Liberation http://www.math.u-bordeaux.fr/~kbelabas/
F-33405 Talence (France) http://pari.math.u-bordeaux.fr/ [PARI/GP]
`

@jdemeyer

This comment has been minimized.

Contributor

jdemeyer commented Mar 23, 2016

What is the intent there ?

No idea, I did not write the deliverable.

@defeo

This comment has been minimized.

Contributor

defeo commented Mar 23, 2016

Ping @vdelecroix. Or maybe @videlec? I'm confused!

@videlec

This comment has been minimized.

Contributor

videlec commented Mar 23, 2016

The interpretation is left to the reader ;-)

  • For item 1 I would say that most of it is done. I remember we had some trouble with the documentation (see comment 13 in Sage ticket #17860). It does not seem to be fixed in sage-7.1.
  • In item 2 it might have been wiser to write "study the possibility of having stack pointers". The gain might actually not be interesting.
@nthiery

This comment has been minimized.

Contributor

nthiery commented Aug 18, 2018

Hi @videlec, @jdemeyer ,
What's the status of this deliverable? Of its report?

We need to submit the report in less than two weeks. Presumably, it can be written cumulatively on top of the previous one, as long as there is a clear highlight of what was achieved since, with an estimate of the manpower spent (after all, if we had known better at writing time, we would have had a single deliverable).
Thanks in advance!
Cheers,

@jdemeyer

This comment has been minimized.

Contributor

jdemeyer commented Aug 21, 2018

Concerning the deliverable itself: features that we added that are not in the first deliverable:

  1. Plotting support in IPython (thanks to the work which was done for #96)

  2. Auto-generating Cython declarations from pari.desc

And in defeo/cypari2#59 I'm working on:

  1. Keep objects on the PARI stack instead of always copying

I haven't yet started with the report.

@jdemeyer

This comment has been minimized.

Contributor

jdemeyer commented Aug 21, 2018

Some less spectacular features that we added:

  1. Python 3 compatibility.

  2. Automatically generate DeprecationWarning for deprecated PARI functions.

  3. Make it compatible with multiple PARI versions (currently the oldest supported is PARI 2.9.0).

  4. Automatic testing using Travis CI and docbuilding on ReadTheDocs.

  5. Support for t_LIST (PARI linked lists).

@nthiery

This comment has been minimized.

Contributor

nthiery commented Aug 23, 2018

Thanks Jeroen for the feedback!

Do you foresee any difficulty in getting the report done early next week to leave enough time for the review? If yes, we can seek for someone to help.

If I understood properly, those are the items in T5.1 that are not yet tackled:

  • Integrate the parallelisation features from T5.1 within PYTHON .
  • Implement a crossed bug report system between SAGE and PARI (using results of T7.3).
    Presumably, we can argue that the latter is out of the scope of this deliverable (though should be tackled one way or the other before the end of the project).

Do you have insight for the former? I don't know what's the current state of the ditto parallelisation features in Pari, and thus how much can be done at this stage anyway. Presumably the report should say something about the status there: maybe reports from experimenting with some of the parallelisation; insight on how they will be integrated, and how much work this will take, if any, ...

@jdemeyer

This comment has been minimized.

Contributor

jdemeyer commented Aug 23, 2018

I don't know what it would mean to "Integrate the parallelisation features from T5.1 within PYTHON". If algorithms in PARI are parallel, then the Python interface will use those parallel features. In other words: this deliverable is about providing an interface to PARI and it shouldn't matter to Python that PARI uses multiple threads.

In the report, we could just explain this and show that it actually works.

There is an issue with cysignals though: that currently does not support multi-threading and it's non-trivial to fix that.

@jdemeyer

This comment has been minimized.

Contributor

jdemeyer commented Aug 23, 2018

Do you foresee any difficulty in getting the report done early next week to leave enough time for the review? If yes, we can seek for someone to help.

I could probably get rough drafts ready for both D4.10 and D4.13 but maybe not polished and finished reports.

@nthiery

This comment has been minimized.

Contributor

nthiery commented Aug 23, 2018

@jdemeyer

This comment has been minimized.

Contributor

jdemeyer commented Aug 29, 2018

Some of the TODOs can only be answered in a negative sense, so I suggest to ignore those.

  • a tiny bit of context to assess how the feature is important: it's not actually important

  • State a few words about the adoption of CyPari2; e.g. is it used by Snappy?: it's only used by Sage as far as I know

@nthiery

This comment has been minimized.

Contributor

nthiery commented Aug 29, 2018

@jdemeyer

This comment has been minimized.

Contributor

jdemeyer commented Aug 29, 2018

I don't really know what to write about

\TODO{The intro of D4.1 says: ``Because of the high degree of
  coupling, and thanks to the availability of Snappy, this deliverable
  constitutes a highly valuable case study for future externalizations
  of low-level interfaces in SageMath.''; a few words about the lesson
  learned while treating this use case would be interesting. Also
  mention the latest workshop in Cernay where you reported on that
  experience.}

I cannot really think of any "lessons learned". But maybe that's a good thing too: everything went fine!

I'm pushing an update now, still some TODOs left.

@embray

This comment has been minimized.

Collaborator

embray commented Aug 29, 2018

@jdemeyer

I cannot really think of any "lessons learned". But maybe that's a good thing too: everything went fine!

Indeed, I think that constitutes a lesson in of itself. In the past some have argued against decoupling parts of Sage, and this is one more piece of evidence that it can be done with no problems and that in the end everyone is (in principle) better off for it.

@jdemeyer

This comment has been minimized.

Contributor

jdemeyer commented Aug 29, 2018

For the latter maybe after the deliverable would be a good time to get in touch with the Snappy devs if not already done?

The main remaining problem is sagemath/cysignals#76

But it looks like we (mostly @vinklein) should be able to fix that.

@embray

This comment has been minimized.

Collaborator

embray commented Aug 29, 2018

To ensure that it remains compatible, multiple Python and \PariGP
versions are tested on the continuous integration platform Travis CI.

In the context of D3.8 (#67) I might be curious if there is anything interesting you learned or problems you encountered using Travis CI for this purpose. I suspect the answer is "nothing special", but just in case you do have anything more to add to that I'd be interested.

@embray

This comment has been minimized.

Collaborator

embray commented Aug 29, 2018

A couple minor edits I had to take or leave as you will (though the original text in the first diff is definitely missing some words, so should be edited somehow or other).

In the second diff I thought that "docstring", being a particular term of art, deserved clarification (even if it has been explained before in other reports).

diff --git a/WP4/D4.10/report.tex b/WP4/D4.10/report.tex
index 33edf348..e94b0cdb 100644
--- a/WP4/D4.10/report.tex
+++ b/WP4/D4.10/report.tex
@@ -69,8 +69,8 @@ This is because 2 threads were used in parallel

 In the first reporting period of the OpenDreamKit project,
 we created \Python bindings for \PariGP in a package called \emph{cypari2}.
-We reported that in \delivref{UI}{pari-python-lib1} and this deliverable is a follow-up.
-we added multiple new features, which we list now.
+We reported on that in \delivref{UI}{pari-python-lib1} and this deliverable is a follow-up
+in which we added multiple new features, which we list here.

 \subsection{Plotting support}

@@ -158,8 +158,9 @@ Out[2]: [x, x^2 + 1]

 \subsection{Documentation}

-\PariGP has a very extensive help for each function.
-This help is now translated to docstrings for the Python interface.
+\PariGP has very extensive help documentation for each function.  This help is
+now translated to ``docstrings'' (in-line documentation for Python code) for
+the Python interface.
 This requires a translation from \LaTeX
 (the language used for the \PariGP documentation)
 to reStructuredText (which is used by Sphinx,
@jdemeyer

This comment has been minimized.

Contributor

jdemeyer commented Aug 29, 2018

In the context of D3.8 (#67) I might be curious if there is anything interesting you learned or problems you encountered using Travis CI for this purpose. I suspect the answer is "nothing special", but just in case you do have anything more to add to that I'd be interested.

From the CI side of things, mostly nothing special. One unfixed issue is that we test a PARI nightly snapshot (in the literal sense, as in a tarball automatically created every night) but that the result is not updated automatically. For example: if I make a commit now to master and you make a PR next month, then that PR might break CI not because of your commit but because something changed in the PARI dependency.

More generally: a wishlist item that I have mostly for cysignals is the ability to test a wide range of platforms with a single CI setup. Right now, it's testing only Ubuntu Linux x86_64. For a while, I regularly had people complaining that cysignals was breaking on some system X but I never had a good way to add automated tests for system X.

@jdemeyer

This comment has been minimized.

Contributor

jdemeyer commented Aug 30, 2018

"reporting FOO" doesn't sound wrong to me but I'll make the change anyway.

Indeed, I think that constitutes a lesson in of itself. In the past some have argued against decoupling parts of Sage, and this is one more piece of evidence that it can be done with no problems and that in the end everyone is (in principle) better off for it.

I'll write something along these lines in the report.

@jdemeyer jdemeyer referenced this issue Aug 30, 2018

Open

T5.1: PARI #99

@embray

This comment has been minimized.

Collaborator

embray commented Aug 30, 2018

Thanks, that's good to know.

That's a tricky problem because most CI systems these days are launching Docker containers, and even with that although you could test "different platforms", when it comes to Cysignals, while some issues will be more relevant to the toolchain used to build it (which can be tested easily with containers), other issues may be relevant to the kernel itself, where containerisation is of no use. So that might be an interesting point to include in my report.

@embray

This comment has been minimized.

Collaborator

embray commented Aug 30, 2018

"reporting FOO" doesn't sound wrong to me but I'll make the change anyway.

In retrospect I think it's fine either way; at first it sounded a bit strange to me though.

@jdemeyer

This comment has been minimized.

Contributor

jdemeyer commented Aug 30, 2018

other issues may be relevant to the kernel itself

This is very much the case with cysignals since it ties in closely with the OS and the C library. The compiler toolchain, CPU or Python version are much less important.

Typically people consider the OS and C library as one whole (from an application programmer point of view, you mostly don't care about what is implemented where). Docker uses the C library from the container, right? At some point, cysignals was crashing on older versions of Debian because of some implementation difference in pthread (which I think was user-space, but I'm not 100% sure). So it might have caught that. But you wouldn't be able to catch OS X problems in a Linux Docker installation.

@jdemeyer

This comment has been minimized.

Contributor

jdemeyer commented Aug 31, 2018

I made some final fixes to the report. For me, it is done.

@KBelabas

This comment has been minimized.

Contributor

KBelabas commented Aug 31, 2018

@nthiery

This comment has been minimized.

Contributor

nthiery commented Aug 31, 2018

Hi @jdemeyer: any chances for you to implement the changes suggested by KB? Worst case I could do it, but I am not sure what to do with the auto generated figures.

@nthiery

This comment has been minimized.

Contributor

nthiery commented Aug 31, 2018

Changes implemented; submitted!

Thanks Jeroen for leading this report :-)

@nthiery nthiery added the Submitted label Aug 31, 2018

@jdemeyer

This comment has been minimized.

Contributor

jdemeyer commented Aug 31, 2018

I'm not sure where the "867 auto-generated functions" comes from

We don't support all functions because we don't support all prototype codes (in particular we don't support closure arguments).

@jdemeyer

This comment has been minimized.

Contributor

jdemeyer commented Aug 31, 2018

More precisely, we do not support prototype codes &, V, I, E, J, C, *, =

@nthiery

This comment has been minimized.

Contributor

nthiery commented Aug 31, 2018

Ah, shoot, too late, the report is submitted. Oh well, it's not that far off. Thanks for the insight though!

@KBelabas

This comment has been minimized.

Contributor

KBelabas commented Sep 1, 2018

@jdemeyer

This comment has been minimized.

Contributor

jdemeyer commented Sep 3, 2018

I see. Any technical reason why not ?

I have not actually tried. We do support closures (more or less, there are details to be fixed), for example apply() is supported. But I never actually looked at what the I, E, J prototype codes mean. If they are literally just passing a t_CLOSURE, then yes it should just work.

@IzabelaFaguet

This comment has been minimized.

Contributor

IzabelaFaguet commented Dec 7, 2018

Accepted by the EU on December 5th.

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