Opened 7 years ago

Closed 6 years ago

#15593 closed enhancement (fixed)

remove sqlalchemy

Reported by: ohanar Owned by:
Priority: major Milestone: sage-6.4
Component: packages: standard Keywords:
Cc: jdemeyer Merged in:
Authors: R. Andrew Ohana, Jeroen Demeyer Reviewers: Vincent Delecroix, Karl-Dieter Crisman, Thierry Monteil
Report Upstream: N/A Work issues:
Branch: 7b582a5 (Commits) Commit: 7b582a5da575000dbacbc5985475403ccd67303a
Dependencies: Stopgaps:

Description (last modified by tmonteil)

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 ohanar

  • Cc jdemeyer added
  • Status changed from new to needs_review

comment:2 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:3 Changed 7 years ago by git

  • Commit changed from b94fd5aab11694e30ef864fa26a06c6216dd8f0a to d701c15354193da3ba02762ca61e6763a74b6b40

Branch pushed to git repo; I updated commit sha1. New commits:

d701c15Merge branch 'trac/develop' into optionalize

comment:4 follow-up: Changed 7 years ago by jdemeyer

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 ohanar

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 ohanar

I don't want to use PyFile_AsFile though since it isn't python 3 compatible.

comment:7 follow-up: Changed 7 years ago by 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)?

comment:8 in reply to: ↑ 7 Changed 7 years ago by ohanar

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 SimonKing

Remark on sqlalchemy: As noted on sage-devel, the current spkg-install of sqlalchemy contains a bug. It says

rm -rf "$SAGE_LOCAL/lib/python/site-packages/SQLAlchemy*"

but should say

rm -rf "$SAGE_LOCAL"/lib/python/site-packages/SQLAlchemy*

as otherwise the old sqlalchemy version will not be removed and will still be used even when installing a new version.

Otherwise, upgrading sqlalchemy-spkg is trivial. Shall I open a separate ticket?

comment:10 follow-up: Changed 7 years ago by jason

If I understand the sqlalchemy spkg files correctly, this spkg-install 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 package---perhaps that's the first external package to be installed...)

Last edited 7 years ago by jason (previous) (diff)

comment:11 in reply to: ↑ 10 Changed 7 years ago by SimonKing

Replying to jason:

(of course, that presumes we have pip as a package---perhaps that's the first external package to be installed...)

That's what someone (actually William, IIRC) has suggested on sage-devel.

comment:12 Changed 7 years ago by jason

Yep, +1 to that. I've wanted pip to be a standard spkg for a long time.

comment:13 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:14 Changed 6 years ago by rws

  • Status changed from needs_review to needs_work
  • Work issues set to rebase

comment:15 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:16 Changed 6 years ago by kcrisman

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 kcrisman

See also https://groups.google.com/forum/#!topic/sage-notebook/LiQEgxT3TWo for removing sqlalchemy altogether.

comment:18 Changed 6 years ago by kcrisman

See also #4268, which is probably invalid at this point.

comment:19 Changed 6 years ago by jdemeyer

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 kcrisman

Probably at this point, yes.

comment:21 Changed 6 years ago by kcrisman

  • 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 sage-devel 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 kcrisman

See #17591 for gdmodule.

comment:23 Changed 6 years ago by tmonteil

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 kcrisman

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 jdemeyer

  • 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 jdemeyer

  • Authors changed from R. Andrew Ohana to R. Andrew Ohana, Jeroen Demeyer
  • Commit changed from d701c15354193da3ba02762ca61e6763a74b6b40 to 2c40f5eb5d5ff3e10c415fbc33778fcb8002a47b
  • Status changed from needs_work to needs_review

I haven't actually tested it, but this should work...


New commits:

2c40f5eRemove sqlalchemy

comment:27 Changed 6 years ago by kcrisman

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 sage-the-distro right now.

comment:28 Changed 6 years ago by tmonteil

  • 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 jdemeyer

make distclean && make ptestlong passed without problems

comment:30 Changed 6 years ago by kcrisman

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 vdelecroix

  • 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 tmonteil

  • Branch changed from u/jdemeyer/ticket/15593 to u/tmonteil/ticket/15593

comment:33 Changed 6 years ago by tmonteil

  • Commit changed from 2c40f5eb5d5ff3e10c415fbc33778fcb8002a47b to 7b582a5da575000dbacbc5985475403ccd67303a
  • Reviewers changed from Vincent Delecroix to Vincent Delecroix, Karl-Dieter Crisman, Thierry Monteil

New commits:

7b582a5#15593 rewiewer comment 27.

comment:34 Changed 6 years ago by vdelecroix

Thanks Thierry. I somehow skip the comment of Karl-Dieter.

comment:35 Changed 6 years ago by vbraun

  • Branch changed from u/tmonteil/ticket/15593 to 7b582a5da575000dbacbc5985475403ccd67303a
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.