Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing commit when building large Cremona database #35050

Merged
merged 1 commit into from
Sep 22, 2024

Conversation

jamesjer
Copy link
Contributor

@jamesjer jamesjer commented Feb 9, 2023

📚 Description

Building the large Cremona database fails like this on an x86_64 Fedora Rawhide machine. (I captured the output from an old sagemath 9.6 build, but the same error occurs with sagemath 9.7.)

Inserting allgens.90000-99999
Committing...
Traceback (most recent call last):
  File "/builddir/build/BUILD/sage-9.6/cremona.sage.py", line 10, in <module>
    c._init_from_ftpdata('ecdata-2019-10-29')
  File "/builddir/build/BUILDROOT/sagemath-9.6-4.fc38.x86_64/usr/lib64/python3.11/site-packages/sage/databases/cremona.py", line 1397, in _init_from_ftpdata
    self.vacuum()
  File "/builddir/build/BUILDROOT/sagemath-9.6-4.fc38.x86_64/usr/lib64/python3.11/site-packages/sage/databases/sql_db.py", line 2180, in vacuum
    self.__connection__.execute('VACUUM')
sqlite3.OperationalError: cannot VACUUM from within a transaction

The issue is a missing database commit, which this PR adds.

📝 Checklist

  • I have made sure that the title is self-explanatory and the description concisely explains the PR.
  • I have linked an issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@github-actions
Copy link

github-actions bot commented Mar 13, 2023

Documentation preview for this PR (built with commit ce90c2d; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@vincentmacri
Copy link
Contributor

@jamesjer If you're still willing to contribute this I'm willing to review it. Can you merge the latest develop branch and also let me know where you got the Cremona database file to build from?

@jamesjer
Copy link
Contributor Author

Okay, I have rebased on top of git head. I got the Cremona database file here, specifically from one of the tags. The last one I tried was 2019-10-29.

@vincentmacri
Copy link
Contributor

vincentmacri commented Sep 13, 2024

I'm having difficulty trying to build the database on 10.4 in order to make sure I can replicate your issue.

Can you explain your process for trying to build the database?

This is what I'm doing that doesn't work:

I'm attempting to use sage.databases.cremona.build and I'm running into errors with extracting the tar file because the name of the tar file isn't ecdata. I get:

Extracting files from /home/[username]/Downloads/ecdata.tar.gz...
tar: ecdata/allcurves: Not found in archive
tar: ecdata/allbsd: Not found in archive
tar: ecdata/degphi: Not found in archive
tar: ecdata/allgens: Not found in archive
tar: Exiting with failure status due to previous errors

Renaming the tar file doesn't work, but extracting it then making a new tarfile called ecdata.tar.gz fixes this problem. Then it complains that I already have a cremona database file so I deleted the database file and now it complains that I don't have a cremona database file.

@jamesjer
Copy link
Contributor Author

I haven't done this for awhile. Looking back through my notes, this seems to be the process. First, run touch /usr/share/sagemath/cremona/cremona.db (replace /usr/share/sagemath with wherever your sagemath installation is). That addresses the problem you are having where it complains that there is no Cremona database file. Then create a file named cremona.sage with these contents:

import sage.databases.cremona
c = sage.databases.cremona.LargeCremonaDatabase('cremona', False, True)
c._init_from_ftpdata('ecdata-2019-10-29')

Replace the ecdata version on that last line with whatever you have. Then run sage cremona.sage.

Copy link
Contributor

@vincentmacri vincentmacri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested locally and everything seems to be working. Thanks for the contribution!

vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 16, 2024
…base

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
### 📚 Description

Building the large Cremona database fails like this on an x86_64 Fedora
Rawhide machine.  (I captured the output from an old sagemath 9.6 build,
but the same error occurs with sagemath 9.7.)
```
Inserting allgens.90000-99999
Committing...
Traceback (most recent call last):
  File "/builddir/build/BUILD/sage-9.6/cremona.sage.py", line 10, in
<module>
    c._init_from_ftpdata('ecdata-2019-10-29')
  File "/builddir/build/BUILDROOT/sagemath-9.6-
4.fc38.x86_64/usr/lib64/python3.11/site-
packages/sage/databases/cremona.py", line 1397, in _init_from_ftpdata
    self.vacuum()
  File "/builddir/build/BUILDROOT/sagemath-9.6-
4.fc38.x86_64/usr/lib64/python3.11/site-
packages/sage/databases/sql_db.py", line 2180, in vacuum
    self.__connection__.execute('VACUUM')
sqlite3.OperationalError: cannot VACUUM from within a transaction
```
The issue is a missing database commit, which this PR adds.

<!-- Describe your changes here in detail -->
<!-- Why is this change required? What problem does it solve? -->
<!-- If it resolves an open issue, please link to the issue here. For
example "Closes sagemath#1337" -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->

- [x] I have made sure that the title is self-explanatory and the
description concisely explains the PR.
- [ ] I have linked an issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies
<!-- List all open pull requests that this PR logically depends on -->
<!--
- #xyz: short description why this is a dependency
- #abc: ...
-->
    
URL: sagemath#35050
Reported by: Jerry James
Reviewer(s): Vincent Macri
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 19, 2024
…base

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
### 📚 Description

Building the large Cremona database fails like this on an x86_64 Fedora
Rawhide machine.  (I captured the output from an old sagemath 9.6 build,
but the same error occurs with sagemath 9.7.)
```
Inserting allgens.90000-99999
Committing...
Traceback (most recent call last):
  File "/builddir/build/BUILD/sage-9.6/cremona.sage.py", line 10, in
<module>
    c._init_from_ftpdata('ecdata-2019-10-29')
  File "/builddir/build/BUILDROOT/sagemath-9.6-
4.fc38.x86_64/usr/lib64/python3.11/site-
packages/sage/databases/cremona.py", line 1397, in _init_from_ftpdata
    self.vacuum()
  File "/builddir/build/BUILDROOT/sagemath-9.6-
4.fc38.x86_64/usr/lib64/python3.11/site-
packages/sage/databases/sql_db.py", line 2180, in vacuum
    self.__connection__.execute('VACUUM')
sqlite3.OperationalError: cannot VACUUM from within a transaction
```
The issue is a missing database commit, which this PR adds.

<!-- Describe your changes here in detail -->
<!-- Why is this change required? What problem does it solve? -->
<!-- If it resolves an open issue, please link to the issue here. For
example "Closes sagemath#1337" -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->

- [x] I have made sure that the title is self-explanatory and the
description concisely explains the PR.
- [ ] I have linked an issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies
<!-- List all open pull requests that this PR logically depends on -->
<!--
- #xyz: short description why this is a dependency
- #abc: ...
-->
    
URL: sagemath#35050
Reported by: Jerry James
Reviewer(s): Vincent Macri
@vbraun vbraun merged commit b3a9253 into sagemath:develop Sep 22, 2024
16 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants