Opened 6 years ago

Closed 4 years ago

#15410 closed enhancement (fixed)

Simplify cythonization of many sage extensions.

Reported by: robertwb Owned by:
Priority: major Milestone: sage-6.8
Component: build Keywords:
Cc: Merged in:
Authors: Robert Bradshaw, Jeroen Demeyer Reviewers: Nathann Cohen, Jean-Pierre Flori
Report Upstream: N/A Work issues:
Branch: 0681b66 (Commits) Commit: 0681b66835d257628a3d2f08a8eadad12d21e846
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

No need to enumerate modules if we don't use special flags.

Change History (31)

comment:1 Changed 6 years ago by robertwb

  • Branch set to u/robertwb/ticket/15410
  • Created changed from 11/13/13 08:20:06 to 11/13/13 08:20:06
  • Modified changed from 11/13/13 08:20:06 to 11/13/13 08:20:06

comment:2 Changed 6 years ago by git

  • Commit set to 94f5a9ffdfd0b3784f75cbac46eef382d7273ea4

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

94f5a9fSimplify cythonization of sage/structure

comment:3 Changed 6 years ago by robertwb

  • Status changed from new to needs_review

comment:4 Changed 6 years ago by git

  • Commit changed from 94f5a9ffdfd0b3784f75cbac46eef382d7273ea4 to 52199847e6ece596afd1ed00bc537cb4c6bb217f

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

5219984Simplify category, coding, and ext cythonization.
708ee36Simplify geometry, games, functions, crypto cythonization.
d5bb198Simplify graphs, groups cythonization.
ee6bff2Simplify cythonization for media, matroids, and misc.

comment:5 Changed 6 years ago by git

  • Commit changed from 52199847e6ece596afd1ed00bc537cb4c6bb217f to 7caecd0ddf6973cb7267f46c2d51198685286ce7

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

7caecd0Trivial cythonization simplification.

comment:6 Changed 6 years ago by robertwb

  • Summary changed from Simplify cythonization of sage/structure. to Simplify cythonization of many sage extensions.

comment:7 Changed 6 years ago by ncohen

Hmmmm... Looks like the 'gmp' dependency in sage.quadratic_forms.count_local_2 was useless ?

Nathann

comment:8 Changed 6 years ago by git

  • Commit changed from 7caecd0ddf6973cb7267f46c2d51198685286ce7 to 7703d9e6accea302f46d66200b0b57a5ae5a9446

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

7703d9eAdd missing gmp library dependency.

comment:9 Changed 6 years ago by robertwb

Eventually this will be inferred from the cimports (e.g. of integer_ring). There's a mess of includes and pxi files that needs to be sorted out before that can happen...

comment:10 Changed 6 years ago by ncohen

  • Reviewers set to Nathann Cohen
  • Status changed from needs_review to positive_review

Hmmmmmm... Well I applied your three patches, deleted the build/ directory, built Sage again and passes all long tests. I got the following errors, and I believe that none of them comes from your commits

----------------------------------------------------------------------
sage -t --long misc/trace.py  # 2 doctests failed
sage -t --long misc/interpreter.py  # 1 doctest failed
sage -t --long combinat/sf/sfa.py  # 1 doctest failed
sage -t --long calculus/desolvers.py  # 8 doctests failed
sage -t --long libs/symmetrica/symmetrica.pxi  # 1 doctest failed
sage -t --long dev/sagedev.py  # 5 doctests failed
sage -t --long dev/patch.py  # 6 doctests failed
sage -t --long tests/cmdline.py  # 14 doctests failed
sage -t --long tests/interrupt.pyx  # Time out
sage -t --long /home/ncohen/.Sage/src/doc/en/constructions/calculus.rst  # 4 doctests failed
sage -t --long /home/ncohen/.Sage/src/doc/en/prep/Quickstarts/Differential-Equations.rst  # 2 doctests failed
----------------------------------------------------------------------
Total time for all tests: 7355.7 seconds
    cpu time: 4916.7 seconds
    cumulative wall time: 5432.8 seconds

Soooooooo well. I'd say "good to go" :-)

Nathann

comment:11 Changed 6 years ago by jdemeyer

  • Authors set to Robert Bradshaw
  • Milestone changed from sage-5.13 to sage-6.0

comment:12 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.0 to sage-6.1

comment:13 Changed 6 years ago by vbraun

  • Status changed from positive_review to needs_work

I'm having problems with this ticket, it works on an incremental build but not in a full rebuild. Dies in conway polynomials: http://build.sagemath.org/sage/builders/%20%20fast%20Volker%20Desktop%20%28Fedora%2019%20x86_64%29%20full/builds/5/steps/compile_1/logs/conway

comment:14 Changed 6 years ago by felixs

while there is no *need* to enumerate files, the proposed wildcarding has been flagged as bad practice outside the sage/distutils world. see http://www.gnu.org/software/automake/manual/html_node/Wildcards.html for an explanation. or just view the explicit listing of files as an extra integrity check similar to docchecks. you get the idea...

python distutils requires a lot of overhead in file lists, which is bad. but wildcards are worse.

this ticket does not improve anything. better leave it like that (YMMV).

comment:15 follow-up: Changed 6 years ago by robertwb

From your link:

Still, these are philosophical objections, and as such you may disagree, or find enough value in wildcards to dismiss all of them. Before you start writing a patch against Automake to teach it about wildcards, let’s see the main technical issue: portability.

You can guess where I fall :).

Portability is not an issue here. The main downside is someone might forget to add/commit the file, but here this is no worse than we are with .py files, and I would say that this inconsistency is bad (.pyx files should be as similar to .py files as possible). Would you propose enumerating the .py files somewhere as an enhancement?

comment:16 in reply to: ↑ 15 Changed 6 years ago by felixs

Replying to robertwb:

Would you propose enumerating the .py files somewhere as an enhancement?

yes.

when i implemented the make/autotools based build system (#15039), i started off with wildcards (which are not a portability issue, since GNU make is a requirement in various other places). only later, i considered integrity/sanity more important than having random files processed/installed/distributed. so I switched over to explicit lists.

unlike distutils, autotools provides some integrity checks ('distcheck') out of the box, i didn't want to miss them. i cant help to note that enhancing/replacing distutils would be far superior to getting rid of the existing file lists.

comment:17 Changed 6 years ago by vbraun

The main reason for autotools requiring explicit file lists is that there is no reliable introspection in C/C++. This is very different in Python. There are certainly pros and cons to wildcards, but just because autotools is doing it doesn't count as an argument IMHO.

comment:18 Changed 6 years ago by felixs

autotools (in combination with GNU make) works with wildcards, but it will not end up smarter than distutils with wildcards. reliable introspection should not be used to decide which files belong to a project.

sage (the library) has tests for every implemented function, no matter how trivisl it is. not checking whether all files are present does not seem to fit. if you are not convinced, go ahead and delete the lists.

comment:19 Changed 6 years ago by robertwb

I'm unable to reproduce this issue with a full rebuild (and your logs are gone). Are you still seeing this?

comment:20 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:21 Changed 6 years ago by jpflori

Should we go on with this ticket? That is use wildcards and move dependency tracking ot the cython file themselves?

comment:22 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:23 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:24 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:25 Changed 5 years ago by leif

  • Milestone changed from sage-6.4 to sage-6.6
  • Type changed from defect to enhancement

comment:26 Changed 4 years ago by jdemeyer

  • Authors changed from Robert Bradshaw to Robert Bradshaw, Jeroen Demeyer
  • Description modified (diff)
  • Milestone changed from sage-6.6 to sage-6.8

I'm rebooting this ticket to handle only the trivial cases (where no additional libraries or compiler flags need to be added).

comment:27 Changed 4 years ago by jdemeyer

  • Branch changed from u/robertwb/ticket/15410 to u/jdemeyer/ticket/15410

comment:28 Changed 4 years ago by jdemeyer

  • Commit changed from 7703d9e6accea302f46d66200b0b57a5ae5a9446 to 0681b66835d257628a3d2f08a8eadad12d21e846
  • Status changed from needs_work to needs_review

New commits:

0681b66Simplify cythonization of many Cython extensions

comment:29 Changed 4 years ago by jpflori

  • Reviewers changed from Nathann Cohen to Nathann Cohen, Jean-Pierre Flori
  • Status changed from needs_review to positive_review

comment:30 Changed 4 years ago by jdemeyer

See #15412 for a very similar ticket.

comment:31 Changed 4 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/15410 to 0681b66835d257628a3d2f08a8eadad12d21e846
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.