Opened 3 months ago

Closed 2 months ago

#28356 closed enhancement (fixed)

Enhanced new build_many to use on all platforms

Reported by: embray Owned by:
Priority: major Milestone: sage-8.9
Component: build Keywords: docbuild parallel pari
Cc: arojas, jdemeyer, fbissey, dimpase, saraedum Merged in:
Authors: Erik Bray Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: e8d26b6 (Commits) Commit: e8d26b6c450c2e9aeac9e78efe64d6e4dbfbe8c7
Dependencies: Stopgaps:

Description

In #27490 I hacked together a replacement for the sage_setup.docbuild.build_many function, which implements (blocking) parallel map() of sorts, which solved some problems with using multiprocessing.Pool.map that stems from its use of threads to fork new processes.

That solved a problem that was specific to older versions of Cygwin. However, there is a similar problem, which affects all platforms, with PARI built with multi-threading support: #26608.

Although the PARI problem begs a more complete solution, at least for the docbuild we can get around it by using the build_many from #27490.

This ticket makes some fixes and enhancements to build_many, so that it can also return a result from the function it runs. For the docbuild this feature is not strictly needed, except for the fact that it can also be used (as in Pool.map) to raise any exception that occurs in one of the worker processes. Thus, it's closer in functionality, at least for the purposes of the docbuild, to Pool.map.

Change History (8)

comment:1 Changed 3 months ago by embray

  • Status changed from new to needs_review

comment:2 Changed 3 months ago by slelievre

  • Status changed from needs_review to needs_work

Branch does not seem to merge.

comment:3 Changed 3 months ago by embray

Well it used to until a day ago...

comment:4 Changed 3 months ago by dimpase

  • Branch changed from u/embray/docbuild/build-many-2 to public/docbuild/build-many-2
  • Commit changed from bd92f138fc24cdccec9a663c1009305c9f92f028 to e8d26b6c450c2e9aeac9e78efe64d6e4dbfbe8c7
  • Status changed from needs_work to needs_review

rebased branch


New commits:

65c7563Enhance the new build_many to actually return results (including exceptions).
e8d26b6Use new build_many on all platforms.

comment:5 Changed 3 months ago by embray

Anyone want to go ahead and try using this?

comment:6 Changed 3 months ago by arojas

Built docs on Arch against system packages including threaded pari, can confirm it no longer segfaults.

comment:7 Changed 3 months ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

looks good to me.

comment:8 Changed 2 months ago by vbraun

  • Branch changed from public/docbuild/build-many-2 to e8d26b6c450c2e9aeac9e78efe64d6e4dbfbe8c7
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.