Skip to content

Remove SAGE_DB from public interface #39012

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Feb 10, 2025
Merged

Remove SAGE_DB from public interface #39012

merged 8 commits into from
Feb 10, 2025

Conversation

tobiasdiez
Copy link
Contributor

The SAGE_DB variable was never used (as far as I can see). So we remove it (as part of the process of migrating the databases to separate python packages).

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

@user202729
Copy link
Contributor

Maybe some user (existing code) are using it? The function db and db_save are exported to the user afte rall.

@orlitzky
Copy link
Contributor

I don't think db() and db_save() have anything to do with the database packages, it looks like they're just shortcuts to pickle/unpickle without providing a path (because it will use SAGE_DB as the path behind the scenes). Seems silly to me to reimplement import pickle in the first place, but whatever.

The comment in the graphs file looks obsolete. And we can certainly remove SAGE_DB from the public interface.

And maybe some day deprecate / separate all of this random stuff that has nothing to do with the rest of sage or mathematics.

@tobiasdiez
Copy link
Contributor Author

Maybe some user (existing code) are using it? The function db and db_save are exported to the user afte rall.

I couldn't find any usage (using github's search). I'm tempted to read sage.misc as "internal".

@orlitzky: Not sure if I should read your comment as a suggestion to change something or as a positive review ;-). Could you clarify what you mean with "The comment in the graphs file looks obsolete."

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 29, 2024 •
edited
Loading

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

@orlitzky
Copy link
Contributor

Could you clarify what you mean with "The comment in the graphs file looks obsolete."

The graphs database is now saved to sage.env.GRAPHS_DATA_DIR, so the comment in isgci.py about SAGE_DB is misleading.

I don't think we should remove db() and db_save() without deprecation. It looks to me like they're thin wrappers around pickling and unpicking, and, even though I think that's pretty pointless, it looks like they were intended for end users at some point.

@tobiasdiez
Copy link
Contributor Author

Could you clarify what you mean with "The comment in the graphs file looks obsolete."

The graphs database is now saved to sage.env.GRAPHS_DATA_DIR, so the comment in isgci.py about SAGE_DB is misleading.

I don't think we should remove db() and db_save() without deprecation. It looks to me like they're thin wrappers around pickling and unpicking, and, even though I think that's pretty pointless, it looks like they were intended for end users at some point.

Okay thanks, I've addressed both points now (db/db_save is now only deprecated)

@tobiasdiez tobiasdiez changed the title Remove SAGE_DB Remove SAGE_DB from public interface Dec 9, 2024
@tobiasdiez tobiasdiez requested a review from orlitzky January 3, 2025 09:38
@tobiasdiez
Copy link
Contributor Author

Thanks @orlitzky. I hope you are okay with me setting it officially to positive review.

vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 31, 2025
61d0fcc 
sagemathgh-39012: Remove SAGE_DB from public interface
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

The `SAGE_DB` variable was never used (as far as I can see). So we
remove it (as part of the process of migrating the databases to separate
python packages).

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [ ] The title is concise and informative.
- [ ] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39012
Reported by: Tobias Diez
Reviewer(s): Michael Orlitzky
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 3, 2025
3f14bcf 
sagemathgh-39012: Remove SAGE_DB from public interface
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

The `SAGE_DB` variable was never used (as far as I can see). So we
remove it (as part of the process of migrating the databases to separate
python packages).

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [ ] The title is concise and informative.
- [ ] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39012
Reported by: Tobias Diez
Reviewer(s): Michael Orlitzky
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 4, 2025
f52f2f8 
sagemathgh-39012: Remove SAGE_DB from public interface
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

The `SAGE_DB` variable was never used (as far as I can see). So we
remove it (as part of the process of migrating the databases to separate
python packages).

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [ ] The title is concise and informative.
- [ ] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39012
Reported by: Tobias Diez
Reviewer(s): Michael Orlitzky
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 9, 2025
8b4a037 
sagemathgh-39012: Remove SAGE_DB from public interface
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

The `SAGE_DB` variable was never used (as far as I can see). So we
remove it (as part of the process of migrating the databases to separate
python packages).

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [ ] The title is concise and informative.
- [ ] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39012
Reported by: Tobias Diez
Reviewer(s): Michael Orlitzky
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 9, 2025
e579c78 
sagemathgh-39012: Remove SAGE_DB from public interface
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

The `SAGE_DB` variable was never used (as far as I can see). So we
remove it (as part of the process of migrating the databases to separate
python packages).

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [ ] The title is concise and informative.
- [ ] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39012
Reported by: Tobias Diez
Reviewer(s): Michael Orlitzky
@vbraun vbraun merged commit cbe236e into sagemath:develop Feb 10, 2025
21 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orlitzky orlitzky orlitzky approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tobiasdiez @user202729 @orlitzky @vbraun