Skip to content
New issue

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

Sign up for GitHub

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

Jump to bottom

Make "./configure --enable-editable" the default #32406

Closed
mkoeppe opened this issue Aug 22, 2021 · 38 comments
Closed

Make "./configure --enable-editable" the default #32406

mkoeppe opened this issue Aug 22, 2021 · 38 comments
Labels
c: build p: major / 3 t: enhancement
Milestone
sage-9.7

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 22, 2021

This has been available since 9.3, has been improved in 9.4 and in the 9.7 series (#33855), and is ready for general consumption

Introduction:

Discussion:

Depends on #32672
Depends on #33855
Depends on #31049
Depends on #33627

CC: @kliem @dimpase @orlitzky @tobiasdiez @saraedum @kiwifb @jhpalmieri @williamstein @nbruin

Component: build

Author: Matthias Koeppe

Branch/Commit: a911e0f

Reviewer: John Palmieri

Issue created by migration from https://trac.sagemath.org/ticket/32406
@mkoeppe mkoeppe added this to the sage-9.5 milestone Aug 22, 2021
@orlitzky
Copy link
Contributor

comment:1

This would avoid the occasional WTF when one forgets to "make" after editing a file, but I've always thought the separation of source and build trees was one of the things that sage got right. Even with modern python projects, you usually want to byte-compile and/or pip-install after changing anything, so the build step is not unexpected. It is tediously slow, though, for sure.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 10, 2021

Dependencies: #32442

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 10, 2021

comment:2

Thanks for the discussion. I'm actually pretty sure that using pip install -e is widely used by Python developers though; but that is probably also connected to the widespread use of virtual environments for specific tasks.

It is true that if one just has one "global" environment (and the monolithic nature of the Sage install with hours of compile time tend to lead to that), the isolation of that environment from the source tree is indeed useful.

So perhaps we need to wait until venv use becomes more popular in Sage developer circles. #32442 is intended to increase venv awareness.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 12, 2021

Changed dependencies from #32442 to #32442, #32672

@mkoeppe mkoeppe modified the milestones: sage-9.5, sage-9.6 Dec 14, 2021
@slel
Copy link
Member

slel commented Dec 29, 2021

comment:5

Remove dependency on #32442, see #29039 comment:163.

@slel
Copy link
Member

slel commented Dec 29, 2021

Changed dependencies from #32442, #32672 to #32672

@mkoeppe mkoeppe modified the milestones: sage-9.6, sage-9.7 Mar 5, 2022
@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 11, 2022

Branch: u/mkoeppe/make____configure___enable_editable__the_default

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 12, 2022

Author: Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 12, 2022

Commit: 01a8f81

@mkoeppe

This comment has been minimized.

Sign in to view
@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 12, 2022

New commits:

01a8f81configure.ac: Make --enable-editable the default

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 12, 2022

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

5fdd8d2README.md: Explain configure --disable-editable

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 12, 2022

Changed commit from 01a8f81 to 5fdd8d2

@tobiasdiez
Copy link
Contributor

comment:11

Shouldn't the editable mode be disabled on some ci workflows so that the so and o files are not included in the docker containers or dist? Or are they excluded by other means?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 12, 2022

comment:12

The so files are needed at runtime

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 12, 2022

comment:13

cc:ing maintainer of the user-facing Docker image

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 15, 2022

Changed dependencies from #32672 to #32672, #33855

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 21, 2022

Changed commit from 5fdd8d2 to ff8710e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 21, 2022

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

8619b29configure.ac: Make --enable-editable the default
6513b6bREADME.md: Explain configure --disable-editable
ff8710edocker/Dockerfile: Use configure --disable-editable

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 21, 2022

comment:16

​https://groups.google.com/g/sage-devel/c/0GV8_O1VwbI (May 2022) only received positive responses, so let's get this in please

@jhpalmieri
Copy link
Member

comment:18

Would it be possible to handle Cython files the same way Python files are handled?

Some timings on my creaky old laptop:

  • with this branch, if I change quaternion_algebra_cython.pyx and run make build: it takes about 70 seconds.
  • with this branch, if I change nothing and run make build, it takes about 60 seconds.
  • with the develop branch, if I change quaternion_algebra_cython.pyx, make build takes about 35 seconds.
  • with the develop branch, if I change nothing, make build takes about 30 seconds.

So this change does have a cost. Presumably it's not as large a difference on a faster computer.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 22, 2022

comment:19

Replying to @jhpalmieri:

Would it be possible to handle Cython files the same way Python files are handled?

No, they need to be compiled

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 22, 2022

comment:20

I'll check where the speed difference is coming from

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 22, 2022

comment:21

I can confirm that it is slower with --enable-editable, but the difference is much smaller on my machine. Typical timings of make sagelib-no-deps:

With ./configure --enable-editable (= new default):

  • no change: make sagelib-no-deps 25.19s user 9.92s system 98% cpu 35.817 total
  • after touch src/sage/algebras/quaternion_algebra_cython.pyx: make sagelib-no-deps 21.77s user 8.55s system 97% cpu 31.039 total
    With ./configure --disable-editable (= same as default on the develop branch):
  • no change: make sagelib-no-deps 16.72s user 7.43s system 91% cpu 26.297 total
  • after touch src/sage/algebras/quaternion_algebra_cython.pyx: make sagelib-no-deps 18.00s user 7.74s system 91% cpu 28.023 total

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 22, 2022

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

7221a76src/setup.py: Do not run find_namespace_packages for 'setup.py dist_info'
a911e0fsrc/MANIFEST.in: prune sage_docbuild, doc

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 22, 2022

Changed commit from ff8710e to a911e0f

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 22, 2022

comment:23

It's faster now

@jhpalmieri
Copy link
Member

comment:24

Yes, it is indeed faster, and the timing is not very different whether I change a single file or don't change anything.

I feel like, in a perfect world, Sage upon startup would automatically detect if it had to recompile because of a change to a Cython file, and conversely, 'sage -b' would be a no-op if no files had been changed. But if that's possible, it's for another ticket.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 22, 2022

comment:25

We can probably add a check for that to display a hint (in a follow-up ticket), but it would be very nonstandard to recompile automatically. I've added this idea to #31406

@mkoeppe

This comment has been minimized.

Sign in to view
@jhpalmieri
Copy link
Member

comment:27

I think a recompile hint would be great!

How many more eyes do we need on this before a positive review? It feels like a major change, but it also feels like it's a simplification.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 22, 2022

comment:28

It's well tested already, and if anything goes wrong there's the easy recourse of using configure --disable-editable

@jhpalmieri
Copy link
Member

Reviewer: John Palmieri

@jhpalmieri
Copy link
Member

comment:29

I'll give it a bit more time in case others want to chime in. If no one responds, I'll set it to positive review later.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 22, 2022

Changed dependencies from #32672, #33855 to #32672, #33855, #31049

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 23, 2022

Changed dependencies from #32672, #33855, #31049 to #32672, #33855, #31049, #33627

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 23, 2022

comment:33

Thanks!

@vbraun
Copy link
Member

vbraun commented Aug 1, 2022

Changed branch from u/mkoeppe/make____configure___enable_editable__the_default to a911e0f

@vbraun vbraun closed this as completed in fd93be8 Aug 1, 2022
kryzar pushed a commit to kryzar/sage that referenced this issue Feb 6, 2023
Trac sagemath#32390: Make "./configure --enable-download-from-upstrea…
f3e9113 
…m-url" the default

This has been available since 9.3 and may be ready for general
consumption

See also: sagemath#32406 Make `./configure --enable-editable` the default

URL: https://trac.sagemath.org/32390
Reported by: mkoeppe
Ticket author(s): Matthias Koeppe
Reviewer(s): Dima Pasechnik
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
c: build p: major / 3 t: enhancement
Projects
None yet
Milestone
sage-9.7
Development

No branches or pull requests
6 participants
@vbraun @orlitzky @jhpalmieri @slel @tobiasdiez @mkoeppe