Opened 3 years ago

Closed 21 months 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)

trac_12341.patch (9.4 KB) - added by ohanar 22 months ago.
re-rebased against 5.4.rc1

Download all attachments as: .zip

Change History (31)

comment:1 Changed 3 years ago by swenson

Also confirmed on my OS X 10.6 with a stock Sage 4.8 install.

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: 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: 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

  • Authors set to R. Andrew Ohana
  • 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: Changed 2 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 2 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 23 months 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 22 months ago by ohanar

ok, patch has been rebased against recent changes (removing SAGE_DATA in favor of SAGE_LOCAL/share)

comment:17 Changed 22 months ago by cremona

Still needs a review from someone, not me!

comment:18 Changed 22 months 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 22 months ago by ohanar

I'm happy with the small changes you made, shall we mark this as positive review?

comment:20 Changed 22 months ago by cremona

  • Status changed from needs_review to positive_review

comment:21 follow-up: Changed 22 months 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 22 months ago by jdemeyer

  • Milestone changed from sage-5.4 to sage-5.5

comment:23 Changed 22 months ago by cremona

  • Description modified (diff)

comment:24 in reply to: ↑ 21 Changed 22 months 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 22 months ago by jdemeyer

  • Status changed from needs_work to positive_review

comment:26 Changed 22 months 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

Changed 22 months ago by ohanar

re-rebased against 5.4.rc1

comment:27 follow-up: Changed 22 months 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 22 months 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 22 months 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 21 months ago by jdemeyer

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