Opened 3 years ago

Closed 2 years ago

#23397 closed enhancement (fixed)

Replace pip2/3-lock with a generic sage-flock command

Reported by: embray Owned by:
Priority: critical Milestone: sage-8.1
Component: build Keywords:
Cc: jdemeyer Merged in:
Authors: Erik Bray Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: b6e4a62 (Commits) Commit: b6e4a62d7d304dd714c0c63ff35c53dc6484d0c5
Dependencies: Stopgaps:

Description

I suggested something like this on #21672 but didn't have immediate need to follow up on it.

Something like this will be useful for making Windows builds more reliable, however, as there are some operations that can fail if they are run simultaneously during parallel builds.

In particular I'm trying to trace down failures with DLL rebasing, and at least part of the problem is that rebase can fail if it's being multiple times simultaneously as can happen during a parallel build (especially toward the end of the build when lots of Python packages are being installed--these are fast and often results in multiple rebases being run simultaneously).

With tickets like #22509 it will also be possible to make further restrictions, such as not running rebase while a package is being uninstalled.

Change History (30)

comment:1 Changed 3 years ago by embray

  • Authors set to Erik Bray
  • Branch set to u/embray/build/lockfile
  • Cc jdemeyer added
  • Commit set to 63f4b2ddae472cc1cf28701486e196d3fe7e1eee
  • Status changed from new to needs_review

(CC: jdemeyer, original author of the script)


New commits:

63f4b2dAdds a new sage-flock command based on the original pip2-lock command, but generalized to

comment:2 Changed 3 years ago by embray

  • Status changed from needs_review to needs_work

Apparently this has a bug...

comment:3 Changed 3 years ago by git

  • Commit changed from 63f4b2ddae472cc1cf28701486e196d3fe7e1eee to 5e89b68a1590856bb4eab77f5e479fd9ac72a17f

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

5e89b68argparse.FileType can be handy, but it doesn't create directories.

comment:4 Changed 3 years ago by embray

  • Status changed from needs_work to needs_review

comment:5 Changed 3 years ago by embray

*ping*

I actually increasingly need this as it's difficult to get through a parallel build on Cygwin now without it failing a few times due to the lack of locking around rebase.

comment:6 follow-up: Changed 3 years ago by jdemeyer

Why did you remove sys.stderr.flush()?

comment:7 Changed 3 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to needs_work

I think it would be good to mention somewhere in a comment that this may be used in build scripts, but only for packages depending on Python.

The help mentions

The command may also be a Python module in which case it is run in the same
Python interpreter as this script (a small optimization to avoid restarting the
Python interpreter).

but I don't see that in the implementation.

comment:8 in reply to: ↑ 6 Changed 3 years ago by jdemeyer

Replying to jdemeyer:

Why did you remove sys.stderr.flush()?

See also https://mail.python.org/pipermail/python-list/2015-November/698421.html

It seems that Python 2 has unbuffered sys.stderr by default, which means that the flushing is not needed.

But this was changed in Python 3, so the flushing would be required there.

comment:9 follow-ups: Changed 3 years ago by embray

Ah, good catch. I had mistakenly assume that the print() function would automatically flush by default.

As for being "only for packages depending on Python" that has me wondering now what the best thing would be to do with this. Maybe it should be moved to build/bin. I need to use it for basically all packages, and the fact that it uses Python is not an impediment to that, since build tools are allowed to use the system Python (and in fact require it). But this script is also used at runtime (just for pip install, currently), in which case it should use Sage's Python. Any ideas?

Last edited 3 years ago by embray (previous) (diff)

comment:10 follow-up: Changed 3 years ago by git

  • Commit changed from 5e89b68a1590856bb4eab77f5e479fd9ac72a17f to e960c8aad05190eda6f4f836ce4c15dd663d82ff

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

60c7670removed false statement from the docstring
e960c8aTurns out print() does not flush I/O by default

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

Replying to embray:

the fact that it uses Python is not an impediment to that, since build tools are allowed to use the system Python (and in fact require it).

If I recall correctly, we require system Python 2.6 or later, which means that you can't use argparse.

comment:12 in reply to: ↑ 10 Changed 3 years ago by jdemeyer

Replying to git:

e960c8aTurns out print() does not flush I/O by default

The flush keyword is Python 3 only.

comment:13 Changed 3 years ago by embray

D'oh, how annoying. In that case I'll just switch it back to sys.stderr.write....

comment:14 in reply to: ↑ 11 ; follow-ups: Changed 3 years ago by embray

Replying to jdemeyer:

Replying to embray:

the fact that it uses Python is not an impediment to that, since build tools are allowed to use the system Python (and in fact require it).

If I recall correctly, we require system Python 2.6 or later, which means that you can't use argparse.

That's not an issue. We already addressed this some time ago... https://git.sagemath.org/sage.git/tree/build/sage_bootstrap/compat?id=3faf8d0519918feb308b3bb56fbe36e0a0c2a290

The question is, should the script live in build/bin or src/bin, and what should go in the shebang line? This is unclear for Python scripts that we want to work at build time and run time...

comment:15 Changed 3 years ago by git

  • Commit changed from e960c8aad05190eda6f4f836ce4c15dd663d82ff to 6e3333d66f82e35d671a99bd5ebde41e60656982

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

6e3333dAnnoyingly, the flush kwarg for the print_function does not exist in Python 2...

comment:16 in reply to: ↑ 9 Changed 3 years ago by jdemeyer

Replying to embray:

I need to use it for basically all packages

What is your use case?

comment:17 Changed 3 years ago by embray

See description.

comment:18 Changed 3 years ago by embray

To clarify, the sage-rebase command opens, and often writes to, every DLL installed in $SAGE_LOCAL. It is run at the end of every package build. If two packages complete build/installation at the same time, then there is a race to run sage-rebase, and the second one can fail, causing the build to halt. This especially happens while installing the Python packages because there are a large number of packages being installed quickly one after the other. But it can happen (and has happened) in principle any other time. So I need to put a lock around sage-rebase to prevent it from being run simultaneously for multiple packages.

Last edited 3 years ago by embray (previous) (diff)

comment:19 in reply to: ↑ 14 ; follow-up: Changed 3 years ago by jdemeyer

Replying to embray:

what should go in the shebang line?

If you intend to use the system Python if applicable, it should be #!/usr/bin/env python.

comment:20 in reply to: ↑ 19 Changed 3 years ago by embray

Replying to jdemeyer:

Replying to embray:

what should go in the shebang line?

If you intend to use the system Python if applicable, it should be #!/usr/bin/env python.

But that's the problem, because if it's intended to be used at runtime it should in that case be using Sage's Python (I guess?), and with SAGE_PYTHON3=yes this is incorrect since python still means python2.7.

That said, the original use case for this was with pip, and running pip is still arguably a "build" action, so maybe this should just be moved to build/bin to emphasize that it's a build tool. It can use the system python by default and/or Sage's Python 2 because ultimately it doesn't matter what Python this is run with since it just execs the program it wraps.

comment:21 Changed 3 years ago by git

  • Commit changed from 6e3333d66f82e35d671a99bd5ebde41e60656982 to 3ec24bc115b2af33c48fe26f27d42eadf38793f7

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

3ec24bcMoved to build/bin to emphasize that this is primarily a build utility (under its current use cases).

comment:22 Changed 3 years ago by embray

  • Status changed from needs_work to needs_review

You've convinced me (and/or I've convinced myself) that this is the best approach.

comment:23 in reply to: ↑ 14 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Replying to embray:

That's not an issue. We already addressed this some time ago... https://git.sagemath.org/sage.git/tree/build/sage_bootstrap/compat?id=3faf8d0519918feb308b3bb56fbe36e0a0c2a290

But then you need to ensure that your script can find this argparse module.

comment:24 Changed 3 years ago by embray

Okay, I was under the mistaken assumption, for some reason, that this was added to PYTHONPATH during builds.

comment:25 Changed 3 years ago by git

  • Commit changed from 3ec24bc115b2af33c48fe26f27d42eadf38793f7 to 18f94549d75c43766b8ec8dc1c3d41a0e0607b9a

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

18f9454Move the contents of sage-flock into the sage_bootstrap package itself, and replace the sage-flock script with a boilerplate script to import and run it from sage_bootstrap

comment:26 Changed 3 years ago by embray

  • Status changed from needs_work to needs_review

comment:27 Changed 3 years ago by git

  • Commit changed from 18f94549d75c43766b8ec8dc1c3d41a0e0607b9a to b6e4a62d7d304dd714c0c63ff35c53dc6484d0c5

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

b6e4a62Merely being part of the sage_bootstrap package doesn't mean we don't still have to manually try to import argparse from the bundled compat module

comment:28 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:29 Changed 3 years ago by embray

  • Priority changed from major to critical

This is increasingly critical since not having it can lead to failures even in the patchbot base build, causing the patchbot to exit.

comment:30 Changed 2 years ago by vbraun

  • Branch changed from u/embray/build/lockfile to b6e4a62d7d304dd714c0c63ff35c53dc6484d0c5
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.