Ticket #8357 (closed defect: fixed)

Opened 3 years ago

Last modified 3 years ago

eclib: suppress / delete files left by doctests and check, fix building in parallel

Reported by: mpatel Owned by: cremona
Priority: major Milestone: sage-4.3.4
Component: elliptic curves Keywords:
Cc: Work issues:
Report Upstream: N/A Reviewers: Minh Van Nguyen
Authors: John Cremona, Mitesh Patel Merged in: sage-4.3.4.alpha1
Dependencies: Stopgaps:

Description (last modified by mpatel) (diff)

From  John Palmieri:

When I run doctests on the file ell_rational_field.py, I end up with a
small file called PRIMES in the current directory. This shouldn't
happen: running doctests shouldn't produce files in a non-temporary
directory.
[...]

This is a follow-up to #7575. Please see comment 24+ for some progress.

A new spkg is available at

It includes the PRIMES, 1, and make check fixes mentioned below and parallel build fixes (cf. #8306).

Note to release manager: Merge only the spkg.

Attachments

trac_8357-eclib_makefiles.patch Download (2.2 KB) - added by mpatel 3 years ago.
Tweak MAKEFILEs for parallel builds. eclib src repo.
trac_8357-spkg.patch Download (1.8 KB) - added by mpatel 3 years ago.
Diff of spkg-install, SPKG.txt. eclib spkg repo.
trac_8357-suppress_PRIMES.patch Download (1.3 KB) - added by mpatel 3 years ago.
Don't write PRIMES. Delete 1 after check. eclib src repo
Makefile Download (17.4 KB) - added by cremona 3 years ago.
replacement src/g0n/Makefile
trac_8357-newforms_dir.patch Download (1.6 KB) - added by mpatel 3 years ago.
Simplify g0n/Makefile. eclib src repo.

Change History

comment:1 Changed 3 years ago by cremona

The quickest quick fix would be just to comment out line 372 in src/procs/marith.cc (in eclib).

Anything else requires thought and testing, which I don't have time for i nthe next several days.

comment:2 Changed 3 years ago by mpatel

  • Status changed from new to needs_review
  • Milestone set to sage-4.3.4
  • Description modified (diff)
  • Authors set to John Cremona, Mitesh Patel

comment:3 follow-up: ↓ 4 Changed 3 years ago by mpatel

  • Status changed from needs_review to needs_work

There's still a timing(?) problem ( log).

Changed 3 years ago by mpatel

Tweak MAKEFILEs for parallel builds. eclib src repo.

comment:4 in reply to: ↑ 3 ; follow-ups: ↓ 5 ↓ 6 Changed 3 years ago by mpatel

Replying to mpatel:

There's still a timing(?) problem ( log).

I think the problem is that make sometimes attempts to build a target in progs before it's done building a requisite shared library, e.g., install_so. I've updated the "makefiles" patch and spkg with a fix that I'm testing now.

comment:5 in reply to: ↑ 4 Changed 3 years ago by cremona

Replying to mpatel:

Replying to mpatel:

There's still a timing(?) problem ( log).

I think the problem is that make sometimes attempts to build a target in progs before it's done building a requisite shared library, e.g., install_so. I've updated the "makefiles" patch and spkg with a fix that I'm testing now.

That sounds likely. We (I) should probably tidy this up; there are several useful executables which are built, but the only one actually used by and accessible from Sage after the build is mwrank. So i should change the targets so that the other progs are only built when doing a make check.

comment:6 in reply to: ↑ 4 Changed 3 years ago by mpatel

Replying to mpatel:

[...] I've updated the "makefiles" patch and spkg with a fix that I'm testing now.

It seems to work and it survives a stress test on sage.math, e.g.,

export MAKE="make -j20"  # And NTL_PREFIX, etc.
make veryclean && make && make so && make veryclean && make && make so && [lots of reps] && ls

comment:7 follow-up: ↓ 8 Changed 3 years ago by cremona

mpatel: if possible could you make he following additional edits to src/procs/Makefile (in eclib...p10):

diff -r 809e34b4c146 procs/Makefile
--- a/procs/Makefile	Sat Feb 27 15:42:31 2010 -0800
+++ b/procs/Makefile	Sun Feb 28 20:08:44 2010 +0000
@@ -105,7 +105,7 @@
 	gzip procs.tar
 
 check: $(TESTS)
-	rm -f PRIMES t
+	rm -f PRIMES t 1
 	./vectest1 < vectest.in >  t && diff t vectest.out
 	./vectest2 < vectest.in >  t && diff t vectest.out
 	./mattest1 < mattest.in >  t && diff t mattest.out
@@ -128,7 +128,7 @@
 	./rcubic < rcubic.in > t && diff t rcubic.out
 	./lcubic < lcubic.in > t && diff t lcubic.out
 	./tp2points < tp2points.in > t && diff t tp2points.out
-	rm -f PRIMES t
+	rm -f PRIMES t 1

It's another temp file which gets left behind, this time after "make check".

If that's too much hassle I'll do it next time I make changes to the eclib spkg, but it would be convenient to do it now.

Changed 3 years ago by mpatel

Diff of spkg-install, SPKG.txt. eclib spkg repo.

Changed 3 years ago by mpatel

Don't write PRIMES. Delete 1 after check. eclib src repo

comment:8 in reply to: ↑ 7 Changed 3 years ago by mpatel

  • Status changed from needs_work to needs_review
  • Description modified (diff)
  • Summary changed from Doctesting ell_rational_field.py leaves a file PRIMES in the current directory to eclib: suppress / delete files left by doctests and check, fix building in parallel

Replying to cremona:

If that's too much hassle I'll do it next time I make changes to the eclib spkg, but it would be convenient to do it now.

Done! I've updated spkg and two patches.

comment:9 Changed 3 years ago by mpatel

I noticed a different problem with make check on t2:

make[3]: Entering directory `/scratch/mpatel/sage-4.3.0.1/spkg/build/eclib-20080310.p10/src/g0n'
rm -f PRIMES t
./modtest < modtest.in > t 2>/dev/null && diff t modtest.out 
./homtest < homtest.in > t && diff t homtest.out
./hecketest < hecketest.in > t 2>/dev/null && diff t hecketest.out 
./mhcount < mhcount.in > t && diff t  mhcount.out
rm -rf tmp_nf_dir
mkdir tmp_nf_dir
export NF_DIR=tmp_nf_dir && ./tmanin < tmanin.in > t 2>/dev/null && diff t tmanin.out
/bin/sh: NF_DIR=tmp_nf_dir: is not an identifier
make[3]: *** [check] Error 1
make[3]: Leaving directory `/scratch/mpatel/sage-4.3.0.1/spkg/build/eclib-20080310.p10/src/g0n'

I think we can fix this with, e.g.,

env NF_DIR=tmp_nf_dir ./tmanin < tmanin.in > t 2>/dev/null && diff t tmanin.out

comment:10 Changed 3 years ago by cremona

I tested the current version of the spkg. (This does nto have the suggested changes to the handling of NF_DIR). All was well.

I did not make new changes (for the NF_DIR thing) since I thought that would be too confusing. But I'll be happy to check the next version of p10 in due course.

comment:11 Changed 3 years ago by cremona

There is a simpler solution to the problem with "export NF_DIR". The code uses a default directory name for this, namely "newforms", which can be over-ridden by the use of the environment variable NF_DIR. For this test therefore we can replace the 2 lines "rm -rf tmp_nf_dir" by "rm -rf newforms" (strictly only the second one is needed) and remove all the starts of lines of the form "export NF_DIR=tmp_nf_dir && ". (This is in src/g0n/Makefile).

Here's why I did not do this in the first place: by own private copy of this code has a newforms directory which contains 3.4G of data which took many many processor-years to compute. I do not want a simple "make check" to delete that so I put in this temporary (and non-default) tmp_nf_dir thing instead. But for the Sage distribution that is not needed.

I can attach a replacement Makefile here if that would be convenient, for yet another version of p10.

comment:12 follow-up: ↓ 13 Changed 3 years ago by mpatel

Sure, that sounds good.

Changed 3 years ago by cremona

replacement src/g0n/Makefile

comment:13 in reply to: ↑ 12 Changed 3 years ago by cremona

Replying to mpatel:

Sure, that sounds good.

I have attached the replacement Makefile -- could you update the spkg with it?

comment:14 Changed 3 years ago by mpatel

  • Description modified (diff)

Done! The package now builds in parallel and make check now works for me on t2 and sage.math.

Changed 3 years ago by mpatel

Simplify g0n/Makefile. eclib src repo.

comment:15 Changed 3 years ago by mvngu

  • Status changed from needs_review to positive_review
  • Reviewers set to Minh Van Nguyen

The updated eclib package  eclib-20080310.p10.spkg looks good. It solves the unexpected annoyance introduced by #7575.

comment:16 Changed 3 years ago by mhansen

  • Status changed from positive_review to closed
  • Resolution set to fixed
  • Merged in set to sage-4.3.4.alpha1
Note: See TracTickets for help on using tickets.