Opened 3 years ago
Closed 2 years ago
#12341 closed defect (fixed)
Empty (full) cremona database in Sage 4.8 causes tests to fail
Reported by: | swenson | Owned by: | cremona |
---|---|---|---|
Priority: | critical | Milestone: | sage-5.5 |
Component: | elliptic curves | Keywords: | |
Cc: | Merged in: | sage-5.5.beta1 | |
Authors: | R. Andrew Ohana | Reviewers: | John Cremona |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by ohanar)
I am on Ubuntu 11.10, and noticed that a fresh install of Sage 4.8 fails a bunch of tests because the full cremona database is an empty file by default.
[swenson@harry 10:18:14] sage-4.8 :) $ ./sage -t /home/swenson/sage-src/sage-4.8/devel/sage-main/sage/misc/functional.py (hg)-[default]- sage -t "devel/sage-main/sage/misc/functional.py" #0: simplify_sum(expr='sum(q^k,k,0,inf)) #1: simplify_sum(expr=a*'sum(q^k,k,0,inf)) ********************************************************************** File "/home/swenson/sage-src/sage-4.8/devel/sage-main/sage/misc/functional.py", line 1360: sage: regulator(EllipticCurve('11a')) Exception raised: Traceback (most recent call last): File "/home/swenson/sage-src/sage-4.8/local/bin/ncadoctest.py", line 1231, in run_one_test self.run_one_example(test, example, filename, compileflags) File "/home/swenson/sage-src/sage-4.8/local/bin/sagedoctest.py", line 38, in run_one_example OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags) File "/home/swenson/sage-src/sage-4.8/local/bin/ncadoctest.py", line 1172, in run_one_example compileflags, 1) in test.globs File "<doctest __main__.example_54[3]>", line 1, in <module> regulator(EllipticCurve('11a'))###line 1360: sage: regulator(EllipticCurve('11a')) File "/home/swenson/sage-src/sage-4.8/local/lib/python/site-packages/sage/schemes/elliptic_curves/constructor.py", line 295, in EllipticCurve return ell_rational_field.EllipticCurve_rational_field(x) File "/home/swenson/sage-src/sage-4.8/local/lib/python/site-packages/sage/schemes/elliptic_curves/ell_rational_field.py", line 198, in __init__ X = sage.databases.cremona.CremonaDatabase()[label] File "/home/swenson/sage-src/sage-4.8/local/lib/python/site-packages/sage/databases/cremona.py", line 1363, in CremonaDatabase _db = LargeCremonaDatabase(name) File "/home/swenson/sage-src/sage-4.8/local/lib/python/site-packages/sage/databases/cremona.py", line 1119, in __init__ + 'not appear to be a valid SQL Cremona database.') RuntimeError: Database at /home/swenson/sage-src/sage-4.8/data//cremona/cremona.db does not appear to be a valid SQL Cremona database. ********************************************************************** 1 items had failures: 1 of 5 in __main__.example_54 ***Test Failed*** 1 failures. For whitespace errors, see the file /home/swenson/.sage//tmp/functional_29532.py [4.3 s] ---------------------------------------------------------------------- The following tests failed: sage -t "devel/sage-main/sage/misc/functional.py" Total time for all tests: 4.3 seconds [swenson@harry 10:18:22] sage-4.8 :( $ rm /home/swenson/sage-src/sage-4.8/data/cremona/cremona.db (hg)-[default]- [swenson@harry 10:18:25] sage-4.8 :) $ ./sage -t /home/swenson/sage-src/sage-4.8/devel/sage-main/sage/misc/functional.py (hg)-[default]- sage -t "devel/sage-main/sage/misc/functional.py" [4.3 s] ---------------------------------------------------------------------- All tests passed! Total time for all tests: 4.3 seconds
See comment:14 for an explanation of the issues causing this.
Apply attachment:trac_12341.patch to the Sage Library.
Attachments (1)
Change History (31)
comment:1 Changed 3 years ago by swenson
comment:2 Changed 3 years ago by was
- Priority changed from major to critical
I notice the spkg-install inside elliptic_curves-0.3/cremona_mini has code like
cremona_root=os.environ['SAGE_DATA']+'/cremona' if not os.path.exists(cremona_root): os.makedirs(cremona_root) target=cremona_root+'/cremona_mini.db'
This is bad form; one should use os.path.join.
comment:3 Changed 3 years ago by was
The following should be using xreadlines, to save memory:
for line in file('src/allcurves.00000-09999').readlines():
This optional test in cremona.py is bad, since it will always fail when the optional package is installed.
sage: c = CremonaDatabase() sage: isinstance(c, sage.databases.cremona.MiniCremonaDatabase) True sage: isinstance(c, sage.databases.cremona.LargeCremonaDatabase) # optional - database_cremona_ellcurve True
Also, the name of the optional component is totally wrong -- it should be elliptic_curves, which is the name of the spkg:
sage-4.8/spkg/standard/elliptic_curves-0.3.spkg
Anyway, trac #11587 unfortunately introduced many issues. Let's fix them asap for 5.0!
comment:4 Changed 3 years ago by ohanar
Just to clarify, this issue occurs without the optional spkg?
comment:5 follow-up: ↓ 7 Changed 3 years ago by swenson
This appeared to be caused by, at some point, I must have run
./sage -t --optional devel/sage/sage/databases/cremona.py
This will create an empty cremona database as a side effect, even though it should not.
comment:6 Changed 3 years ago by swenson
ohanar, yes -- I did not install any optional packages.
comment:7 in reply to: ↑ 5 Changed 3 years ago by ohanar
Replying to swenson:
This appeared to be caused by, at some point, I must have run
./sage -t --optional devel/sage/sage/databases/cremona.py
This will create an empty cremona database as a side effect, even though it should not.
I think the culprit is line 1088:
sage: c = CremonaDatabase('cremona') # optional
The issue is because I didn't check for read_only=False before opening a non-existent database. Shouldn't be too hard to fix.
-- Andrew
comment:8 Changed 3 years ago by ohanar
- Status changed from new to needs_review
Hopefully that fixes this issue. Still a lot of things that could be improved as William noted.
comment:9 follow-up: ↓ 10 Changed 3 years ago by cremona
As one of the reviewers for #11587, I apologise. I'm not sure what I should have done differently, other than being clever when reading the new code there. This was rather a complicated change to make since both before and after there were standard and optional spkgs involved. I cannot have tested all possible combinations. At least now we only have two combinations to check (before and after installing the optional large database). All the 4.8 builds I have already have the large database installed though, so unless someone tells me how to uninstall it I cannot test the effect of this patch!
comment:10 in reply to: ↑ 9 Changed 3 years ago by ohanar
I realized that my patch only half fixes the problem, the creation of and empty full database. The other half is for it to not use that file unless it sees that the full database is installed. I most likely won't be able to get around to this today.
Replying to cremona:
As one of the reviewers for #11587, I apologise.
This was rather a complicated change to make since both before and after there were standard and optional spkgs involved.
Yeah, I'm not really surprised by bugs popping up, it was a major change.
comment:11 Changed 3 years ago by ohanar
- Status changed from needs_review to needs_work
comment:12 Changed 3 years ago by ohanar
- Status changed from needs_work to needs_review
Ok, I've updated the patch. The optional spkg is here if you would like to test with it (the spkg repo hasn't been updated yet).
comment:13 follow-up: ↓ 14 Changed 3 years ago by cremona
- Reviewers set to John Cremona
Patch applies fine and all elliptic_curves tests pass both before and after installing the optional database (which with 5.0.bets5 I installed with a plain "sage -i database_cremona_ellcurve").
I have not checked all the logic in the revised code. I was puzzled by the remark about testing with flag --optional since I thought that only made sense with some sensible string after the word "optional", e.g. "magma". But it seems that there are doctest lines which are tagged "# optional" with no mention of which optional package is relevant. Possibly for this reason, I had weird failures with
sage -t --optional devel/sage-main/sage/schemes/elliptic_curves/lseries_ell.py # 3 doctests failed
which may have nothing to do with this ticket.
I hesitate to set this to Positive Review since I really don't understand what it going on, but at least I have done something and reported what has happened.
comment:14 in reply to: ↑ 13 Changed 3 years ago by ohanar
- Description modified (diff)
Replying to cremona:
I have not checked all the logic in the revised code.
I don't think I have explained the issue that this ticket fixes very clearly (which was exposed when running one of the optional doctests without having the optional spkg installed). There are 2 main issues that caused this:
- When calling CremonaDatabase('name') the file SAGE_ROOT/data/cremona/name.db was being touched, something that is bad in general if the file wasn't there to start with.
- When calling CremonaDatabase(), to determine if the full database is installed, it would check for the existence of SAGE_ROOT/data/cremona/cremona.db rather than use one of the functions in sage.misc for this purpose.
With these combined, running the optional doctests would break the cremona database: it would run CremonaDatabase('cremona'), which would touch SAGE_ROOT/data/cremona/cremona.db and thus later calls of CremonaDatabase() would break since it would think the full cremona database was installed and try to use the touched file.
I was puzzled by the remark about testing with flag --optional since I thought that only made sense with some sensible string after the word "optional", e.g. "magma".
Unless you want to test all optional packages (which could be useful for a giant system install -- unfortunately there are a few optional packages that don't want to build properly at the moment).
But it seems that there are doctest lines which are tagged "# optional" with no mention of which optional package is relevant.
I consider this a bug and something that should be required for any optional tags.
comment:15 Changed 3 years ago by cremona
- Description modified (diff)
The second patch (which includes the changes in the previous one) addresses the cases where there is a tag #optional in the code but no indication of which optional pakage is required, for the elliptic_curves directory only.
With sage-5.4.beta1 with no optional spkgs installed, all tests pass if -optional is not given, while if -optional is given then there are two failures in elliptic_curve/, both due to {{{database_cremona_ellcurve}} not being installed. After installing that one optional spkg, all these pass. That is as it should be.
Since I added to the patch it needs a second referee.
comment:16 Changed 2 years ago by ohanar
ok, patch has been rebased against recent changes (removing SAGE_DATA in favor of SAGE_LOCAL/share)
comment:17 Changed 2 years ago by cremona
Still needs a review from someone, not me!
comment:18 Changed 2 years ago by cremona
I can confirm that trac_12341.patch applies cleanly to 5.4.rc1 and all tests in sage/schemes/elliptic_curves still pass.
Andrew, if you are happy with the (very small) changes I had made to your first patch, between us we can make this a positive review.
Note that #12565 depends on this...
comment:19 Changed 2 years ago by ohanar
I'm happy with the small changes you made, shall we mark this as positive review?
comment:20 Changed 2 years ago by cremona
- Status changed from needs_review to positive_review
comment:21 follow-up: ↓ 24 Changed 2 years ago by jdemeyer
- Status changed from positive_review to needs_work
#optional - database_cremona_ellcurve (conductor is greater than 10000)
should be
# optional - database_cremona_ellcurve
(you're not supposed to put extra stuff after "optional")
comment:22 Changed 2 years ago by jdemeyer
- Milestone changed from sage-5.4 to sage-5.5
comment:23 Changed 2 years ago by cremona
- Description modified (diff)
comment:24 in reply to: ↑ 21 Changed 2 years ago by cremona
Replying to jdemeyer:
#optional - database_cremona_ellcurve (conductor is greater than 10000)should be
# optional - database_cremona_ellcurve(you're not supposed to put extra stuff after "optional")
Fixed -- please may I have my positive review back now?
comment:25 Changed 2 years ago by jdemeyer
- Status changed from needs_work to positive_review
comment:26 Changed 2 years ago by jdemeyer
- Status changed from positive_review to needs_work
I'm afraid this needs to be rebased to sage-5.4.rc1:
applying trac_12341a.patch patching file sage/databases/cremona.py Hunk #1 FAILED at 51 Hunk #2 FAILED at 113 Hunk #4 FAILED at 593 Hunk #7 FAILED at 1317 Hunk #9 FAILED at 1567 5 out of 9 hunks FAILED -- saving rejects to file sage/databases/cremona.py.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh trac_12341a.patch
comment:27 follow-up: ↓ 28 Changed 2 years ago by ohanar
- Description modified (diff)
- Status changed from needs_work to needs_review
ok, re-rebased against 5.4.rc1 (it seems that my rebased changes disappeared when John went to fix the optional doctest flag)
comment:28 in reply to: ↑ 27 Changed 2 years ago by cremona
Replying to ohanar:
ok, re-rebased against 5.4.rc1 (it seems that my rebased changes disappeared when John went to fix the optional doctest flag)
Very sorry, that was entirely my fault as your rebased patch had the same name as the previous one; I had the old patch on my machine and just edited the patch file without downloading the new version.
comment:29 Changed 2 years ago by cremona
- Status changed from needs_review to positive_review
There is one small remaining issue here, which admittedly is not caused by theis ticket/patch. Namely, all tests pass with this patch without the optional database installed, but when it is installed there is a doctest error on line 6454 of elliptic_curves/heegner.py. That needs to be fixed somewhere, so that it passes whether or not the optional db is installed. But I am not sure how to do that: it is an example to illustrate a specific behaviour, so the usual trick of using a curve whose conductor is so large it will not be in the optional database in the forseeable future will not do.
It seems bad to hold up this ticket for this reason! So I am giving this a positive review, and refer to #13547 to address the remaining issue.
comment:30 Changed 2 years ago by jdemeyer
- Merged in set to sage-5.5.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
Also confirmed on my OS X 10.6 with a stock Sage 4.8 install.