Opened 7 years ago
Closed 6 years ago
#15593 closed enhancement (fixed)
remove sqlalchemy
Reported by:  ohanar  Owned by:  

Priority:  major  Milestone:  sage6.4 
Component:  packages: standard  Keywords:  
Cc:  jdemeyer  Merged in:  
Authors:  R. Andrew Ohana, Jeroen Demeyer  Reviewers:  Vincent Delecroix, KarlDieter Crisman, Thierry Monteil 
Report Upstream:  N/A  Work issues:  
Branch:  7b582a5 (Commits)  Commit:  7b582a5da575000dbacbc5985475403ccd67303a 
Dependencies:  Stopgaps: 
Description (last modified by )
It is not used in sage nor sagenb and can easily be installed with pip.
Change History (35)
comment:1 Changed 7 years ago by
 Cc jdemeyer added
 Status changed from new to needs_review
comment:2 Changed 7 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:3 Changed 7 years ago by
 Commit changed from b94fd5aab11694e30ef864fa26a06c6216dd8f0a to d701c15354193da3ba02762ca61e6763a74b6b40
comment:4 followup: ↓ 5 Changed 7 years ago by
I dislike
fn = open(filename, 'wb') # check filename fn.close() cdef FILE *out = fopen(filename, 'wb') gdImagePng(im, out) fclose(out)
You can open the file using Python and access its FILE*
with PyFile_AsFile.
comment:5 in reply to: ↑ 4 Changed 7 years ago by
Replying to jdemeyer:
I dislike ...
Sure, I didn't really rethink any of the code, just refactored what was already there a bit.
comment:6 Changed 7 years ago by
I don't want to use PyFile_AsFile
though since it isn't python 3 compatible.
comment:7 followup: ↓ 8 Changed 7 years ago by
Since I need sqlalchemy (or at least *some* database), I'll not review it. Just some questions: Is there the plan to turn the current standard sqlalchemy spkg into an optional one? Will the current standard spkg (version 0.5.8) be upgraded when being made an optional spkg (version 0.9.3)?
comment:8 in reply to: ↑ 7 Changed 7 years ago by
Replying to SimonKing:
Since I need sqlalchemy (or at least *some* database), I'll not review it. Just some questions: Is there the plan to turn the current standard sqlalchemy spkg into an optional one? Will the current standard spkg (version 0.5.8) be upgraded when being made an optional spkg (version 0.9.3)?
I've not removed sqlalchemy from the repository, so it will remain as an optional spkg (although not upgraded, probably a trivial upgrade though). sqlalchemy is not a database, but rather a wrapper for sql databases to give them a unified interface. We currently ship sqlite as our database of choice.
comment:9 Changed 7 years ago by
Remark on sqlalchemy: As noted on sagedevel, the current spkginstall of sqlalchemy contains a bug. It says
rm rf "$SAGE_LOCAL/lib/python/sitepackages/SQLAlchemy*"
but should say
rm rf "$SAGE_LOCAL"/lib/python/sitepackages/SQLAlchemy*
as otherwise the old sqlalchemy version will not be removed and will still be used even when installing a new version.
Otherwise, upgrading sqlalchemyspkg is trivial. Shall I open a separate ticket?
comment:10 followup: ↓ 11 Changed 7 years ago by
If I understand the sqlalchemy spkg files correctly, this spkginstall does nothing more than a standard "python setup.py install". Over the years, there have been a fair number of such packages, or at least requests for such packages. How about we make "sage i" try to "pip install" a package if it can't find the corresponding spkg? Then "standard python installs" don't need to be maintained as spkgs, but people still have a transparent way to install them.
(of course, that presumes we have pip as a packageperhaps that's the first external package to be installed...)
comment:11 in reply to: ↑ 10 Changed 7 years ago by
Replying to jason:
(of course, that presumes we have pip as a packageperhaps that's the first external package to be installed...)
That's what someone (actually William, IIRC) has suggested on sagedevel.
comment:12 Changed 7 years ago by
Yep, +1 to that. I've wanted pip to be a standard spkg for a long time.
comment:13 Changed 6 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:14 Changed 6 years ago by
 Status changed from needs_review to needs_work
 Work issues set to rebase
comment:15 Changed 6 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:16 Changed 6 years ago by
pip is in the meantime standard. See also #8757. Where is sqlalchemy used, anyway (in the Sage library, I mean, not disregarding Simon's point).
comment:17 Changed 6 years ago by
See also https://groups.google.com/forum/#!topic/sagenotebook/LiQEgxT3TWo for removing sqlalchemy altogether.
comment:18 Changed 6 years ago by
See also #4268, which is probably invalid at this point.
comment:19 Changed 6 years ago by
Shouldn't "sqlalchemy should not be a standard package" and "gdmodule should not be a standard package" be 2 different tickets?
comment:20 Changed 6 years ago by
Probably at this point, yes.
comment:21 Changed 6 years ago by
 Summary changed from sqlalchemy and gdmodule should not be standard packages to sqlalchemy should not be standard package
By the way, sqlalchemy was added in #2205 and this sagedevel thread, where no one ever actually needed it! It was just ... useful. But apparently not enough so to use it for any of our databases.
comment:22 Changed 6 years ago by
See #17591 for gdmodule.
comment:23 Changed 6 years ago by
I am finishing a kind of audit of packages in Sage, and noticed sqlalchemy was useless (and bitrotting for a while). Since there is no interaction with Sage (nor sagenb), i think removing it is a better option than updating it and make it optional. There will be less work to do, and sage pip install sqlalchemy
will let the user have the latest version (or even any other version if needed).
comment:24 Changed 6 years ago by
That's fine, I just figured people who know more about this than I do should be the ones to change it to 'remove' :)
comment:25 Changed 6 years ago by
 Branch changed from u/ohanar/optionalize to u/jdemeyer/ticket/15593
 Created changed from 12/26/13 23:07:10 to 12/26/13 23:07:10
 Modified changed from 01/06/15 16:01:30 to 01/06/15 16:01:30
comment:26 Changed 6 years ago by
 Commit changed from d701c15354193da3ba02762ca61e6763a74b6b40 to 2c40f5eb5d5ff3e10c415fbc33778fcb8002a47b
 Status changed from needs_work to needs_review
comment:27 Changed 6 years ago by
I would actually not get rid of the line in the db file, since (in principle) someone ambitious could do that someday and it's only in a todo list. We aren't saying we hate sqlalchemy, just that there is no reason to include it in sagethedistro right now.
comment:28 Changed 6 years ago by
 Description modified (diff)
 Summary changed from sqlalchemy should not be standard package to remove sqlalchemy
 Work issues rebase deleted
comment:29 Changed 6 years ago by
make distclean && make ptestlong
passed without problems
comment:30 Changed 6 years ago by
I don't really care about this so much, but just be sure that sagenb does indeed work fine (though I have no reason to expect it not to!).
comment:31 Changed 6 years ago by
 Reviewers set to Vincent Delecroix
 Status changed from needs_review to positive_review
I just rebuild Sage, testall, reinstall the notebook and ran it...
Vincent
comment:32 Changed 6 years ago by
 Branch changed from u/jdemeyer/ticket/15593 to u/tmonteil/ticket/15593
comment:33 Changed 6 years ago by
 Commit changed from 2c40f5eb5d5ff3e10c415fbc33778fcb8002a47b to 7b582a5da575000dbacbc5985475403ccd67303a
 Reviewers changed from Vincent Delecroix to Vincent Delecroix, KarlDieter Crisman, Thierry Monteil
New commits:
7b582a5  #15593 rewiewer comment 27.

comment:34 Changed 6 years ago by
Thanks Thierry. I somehow skip the comment of KarlDieter.
comment:35 Changed 6 years ago by
 Branch changed from u/tmonteil/ticket/15593 to 7b582a5da575000dbacbc5985475403ccd67303a
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Merge branch 'trac/develop' into optionalize