Opened 7 years ago
Closed 7 years ago
#19187 closed enhancement (fixed)
Add rules for installing packages with pip
Reported by: | jdemeyer | Owned by: | |
---|---|---|---|
Priority: | critical | Milestone: | sage-6.9 |
Component: | build | Keywords: | |
Cc: | Merged in: | ||
Authors: | Jeroen Demeyer | Reviewers: | Vincent Delecroix, Volker Braun |
Report Upstream: | N/A | Work issues: | |
Branch: | 52a3cf0 (Commits, GitHub, GitLab) | Commit: | 52a3cf08a5c485ecc5b080ac19f7e5080ac4fc06 |
Dependencies: | Stopgaps: |
Description (last modified by )
With the infrastructure of #19101, we can make it such that
sage -i pyopenssl
actually runs pip
.
All the following optional packages need to be removed from the server as they are now supported by the above command:
- beautifulsoup
- biopython
- brian
- guppy
- mercurial
- mpi4py
- nibabel
- pybtex
- pyflakes
- pyopenssl
- sqlalchemy
- trac
Change History (44)
comment:1 Changed 7 years ago by
- Branch set to u/jdemeyer/ticket/19187
comment:2 Changed 7 years ago by
- Commit set to adef308eaf6a6c478301471c3c2350260b39a5fe
- Status changed from new to needs_review
comment:3 Changed 7 years ago by
- Priority changed from major to critical
comment:4 follow-up: ↓ 5 Changed 7 years ago by
Can we do that for all packages that are installable that way (brian, biopython, etc.) then? (I tried to look at the commit to see if it did that but my internet is very flaky until I get to work.)
comment:5 in reply to: ↑ 4 Changed 7 years ago by
Replying to kcrisman:
Can we do that for all packages that are installable that way?
The current branch deals with
beautifulsoup biopython brian guppy mpi4py nibabel pybtex pyflakes pyopenssl trac
comment:6 Changed 7 years ago by
Thanks - I actually saw that just before my internet cut out on the train :-( so I couldn't edit my post - but I appreciate the quick followup :-) and good thinking to do this, of course, we should have long ago.
comment:7 Changed 7 years ago by
- Commit changed from adef308eaf6a6c478301471c3c2350260b39a5fe to fdba3ee0772b1885c3f60cb35f850fee576a7f9a
Branch pushed to git repo; I updated commit sha1. New commits:
fdba3ee | Add mercurial and sqlalchemy (ex-standard packages)
|
comment:8 follow-up: ↓ 9 Changed 7 years ago by
(You know, this could be very helpful for disentangling the many packages in sagenb.)
comment:9 in reply to: ↑ 8 Changed 7 years ago by
Replying to kcrisman:
(You know, this could be very helpful for disentangling the many packages in sagenb.)
Actually not, because these special pip
rules are not standard packages.
comment:10 follow-ups: ↓ 12 ↓ 13 Changed 7 years ago by
Hi Jeroen
Do you consider your solution as temporary? I am afraid that it would become an open door to have thousands of packages in piprules
. Shouldn't we reorient users toward sage -pip install X
?
Vincent
comment:11 follow-up: ↓ 14 Changed 7 years ago by
Could this also be modified to work for nzmath
and pyx
?
comment:12 in reply to: ↑ 10 Changed 7 years ago by
Do you consider your solution as temporary? I am afraid that it would become an open door to have thousands of packages in
piprules
.
That could be regarded as a feature. After all, we'd still want to have some test with respect to doctests or something. If people just want to add a pip package, whatever. If they want to add a documentation file with interactions with standard Sage, why not? Or is it just a flood waiting to get in?
comment:13 in reply to: ↑ 10 ; follow-up: ↓ 17 Changed 7 years ago by
Replying to vdelecroix:
Do you consider your solution as temporary?
No.
I am afraid that it would become an open door to have thousands of packages in
piprules
.
which is problem because...? Besides, we didn't have "thousands" of optional/experimental packages before, so I don't see why this would suddenly become thousands now.
Shouldn't we reorient users toward
sage -pip install X
?
I don't think so, because how is the user supposed to know if he should use sage -i PKG
or sage --pip install PKG
? With my patch here, the user just needs to know sage -i PKG
.
comment:14 in reply to: ↑ 11 Changed 7 years ago by
Replying to tscrim:
Could this also be modified to work for
nzmath
andpyx
?
If you successfully installed any of those two using pip
, please tell me how. The obvious sage --pip install pyx
or sage --pip install nzmath
does not work.
comment:15 Changed 7 years ago by
Vincent was able to install them, although you need to pass additional arguments to pip
. See this thread. I will try later today to see if I can get those to work.
However, perhaps nzmath should (continue to) be a spkg as the latest version is 1.2 and pip would install 1.1?
comment:16 Changed 7 years ago by
Seems to not work anymore:
sage -pip install --allow-external pyx --allow-unverified pyx pyx
ended up with
No files/directories in /tmp/pip-build-d6xYvF/pyx/pip-egg-info (from PKG-INFO)
and
sage -pip install --allow-external nzmath --allow-unverified nzmath nzmath
ended up with some timeout
comment:17 in reply to: ↑ 13 Changed 7 years ago by
- Status changed from needs_review to needs_work
I just tested all the list of packages provied in comment:5 and everything worked fine.
Replying to jdemeyer:
Replying to vdelecroix:
Do you consider your solution as temporary?
No.
Then it should be complient with sage --optional
which lists the packages. Right now (after installing all the packages supported) I got:
$ sage --optional beautifulsoup........................... 3.2.1 (not_installed) biopython............................... 1.61 (not_installed) brian................................... 1.4.1.p0 (not_installed) mpi4py.................................. 1.2.2 (not_installed) trac.................................... 0.11.5.p0 (not_installed) etc
And also with sage -t
that uses optional packages.
$ sage -t whatever ... Using --optional=cbc,mpir,python2,sage,scons ...
I am afraid that it would become an open door to have thousands of packages in
piprules
.which is problem because...? Besides, we didn't have "thousands" of optional/experimental packages before, so I don't see why this would suddenly become thousands now.
Because it becomes now very easy. But you might be right.
Shouldn't we reorient users toward
sage -pip install X
?I don't think so, because how is the user supposed to know if he should use
sage -i PKG
orsage --pip install PKG
? With my patch here, the user just needs to knowsage -i PKG
.
No. And as Karl-Dieter said it would be good to add tests for them # optional - biopython
.
Vincent
comment:18 follow-up: ↓ 24 Changed 7 years ago by
Sorry Vincent, but that's currently too much to ask. It's hard to make these "real" packages without major changes to the package system.
So either this ticket get accepted as-is (potentially with minor modifications) or closed as "wontfix".
comment:19 follow-up: ↓ 23 Changed 7 years ago by
Think of these packages as forming a new category: we have standard packages, optional packages, experimental packages and pip packages. They are not optional packages and cannot be listed by some sage --whatever
command.
comment:20 Changed 7 years ago by
Although we can check existing optional packages to see if they've been installed by pip:
-
src/bin/sage-list-packages
diff --git a/src/bin/sage-list-packages b/src/bin/sage-list-packages index e809e40..9162a30 100755
a b 5 5 6 6 from __future__ import print_function 7 7 8 import os, sys, argparse, re8 import os, argparse, re, subprocess 9 9 from string import join 10 10 11 11 if "SAGE_ROOT" not in os.environ: … … if args['category'] == 'installed': 98 98 # Any other categories 99 99 else: 100 100 if args['version']: 101 try: 102 pip_output = subprocess.check_output(['pip', 'list'], stderr=subprocess.STDOUT) 103 except CalledProcessError: 104 pip_output = '' 105 101 106 # Display the columns' name 102 107 if not args['dump']: 103 108 print("{:.<40} {} ({})\n".format("[package] ", "[latest version]", 104 109 "[installed version]")) 105 110 for p in sorted(set(local).union(remote)): 111 if p in installed: 112 installed_version = installed[p] 113 else: 114 s = re.search('\n{}.*\((.*)\)\n'.format(p), pip_output) 115 if s: 116 installed_version = s.groups(0)[0] 117 else: 118 installed_version = 'not_installed' 106 119 line = [p, local.get(p, remote.get(p, 'not_found')), 107 installed .get(p, 'not_installed')]120 installed_version] 108 121 if args['dump']: 109 122 print(join(line, ' ')) 110 123 else:
(This is not a complete patch, just an idea.)
comment:21 Changed 7 years ago by
And if we think this sort of patch is a good idea, it can go on a later ticket, not this one.
comment:22 Changed 7 years ago by
See #19213.
comment:23 in reply to: ↑ 19 Changed 7 years ago by
Replying to jdemeyer:
Think of these packages as forming a new category: we have standard packages, optional packages, experimental packages and pip packages. They are not optional packages and cannot be listed by some
sage --whatever
command.
What is this piprules file then!? It is possible to check the last version of a package with their json api
import urllib2 import json def pip_versions(package_name): url = "https://pypi.python.org/pypi/{}/json".format(package_name) try: data = json.load(urllib2.urlopen(urllib2.Request(url))) except urllib2.HTTPError: return [] else: versions = data["releases"].keys() return versions
comment:24 in reply to: ↑ 18 Changed 7 years ago by
Replying to jdemeyer:
Sorry Vincent, but that's currently too much to ask. It's hard to make these "real" packages without major changes to the package system.
So either this ticket get accepted as-is (potentially with minor modifications) or closed as "wontfix".
I think this ticket is good, but it lacks integration. I would be also happy if all of brian
, trac
, ... would not appear in sage --optional
.
Vincent
comment:25 follow-up: ↓ 26 Changed 7 years ago by
In other words, we could:
- remove all of
brian
,trac
, ... from the list of optional packages - postponed the pip listing and test support to #19213
what do you think?
comment:26 in reply to: ↑ 25 Changed 7 years ago by
Replying to vdelecroix:
In other words, we could:
- remove all of
brian
,trac
, ... from the list of optional packages- postponed the pip listing and test support to #19213
what do you think?
Fine for me. Do you give positive to this ticket then?
comment:27 follow-up: ↓ 28 Changed 7 years ago by
- Description modified (diff)
- Reviewers set to Vincent Delecroix
- Status changed from needs_work to positive_review
Should we explicit ask on sage-devel for removing them from the server?
Vincent
comment:28 in reply to: ↑ 27 Changed 7 years ago by
Replying to vdelecroix:
Should we explicit ask on sage-devel for removing them from the server?
See #19220.
comment:29 follow-ups: ↓ 31 ↓ 36 Changed 7 years ago by
- Status changed from positive_review to needs_work
Doesn't work
$ sage -f brian [... endless noise ...] sage --pip uninstall -y brian You are using pip version 6.1.1, however version 7.1.2 is available. You should consider upgrading via the 'pip install --upgrade pip' command. Cannot uninstall requirement brian, not installed Makefile:2244: recipe for target 'brian-clean' failed
Also breaks the buildbot script that assumes that build/pkgs/*
has a corresponding log file etc. IMHO one dirent for one package is IMHO a better organization, stuffing multiple packages into lines of a single file doesn't scale (merge conflicts if its too popular).
comment:30 follow-up: ↓ 34 Changed 7 years ago by
This might be a little ugly:
-
build/make/install
diff --git a/build/make/install b/build/make/install index 7469661..a164fc4 100755
a b while read PKG_NAME; do 473 473 474 474 # Add a target to remove the "installed" file for "sage -f" 475 475 echo >&5 "$PKG_NAME-clean:" 476 echo >&5 " sage --pip uninstall -y $PKG_NAME"476 echo >&5 " sage --pip list | grep $PKG_NAME && sage --pip uninstall -y $PKG_NAME || :" 477 477 echo >&5 478 478 done <"$SAGE_ROOT/build/pkgs/piprules" 479 479
Or the same but omitting sage --pip list | grep $PKG_NAME &&
.
comment:31 in reply to: ↑ 29 Changed 7 years ago by
Replying to vbraun:
Also breaks the buildbot script that assumes that
build/pkgs/*
has a corresponding log file etc. IMHO one dirent for one package is IMHO a better organization, stuffing multiple packages into lines of a single file doesn't scale (merge conflicts if its too popular).
An alternate version could introduce a new package type ("pip"?), with one directory for each of these packages, just containing type
and SPKG.txt
.
comment:32 Changed 7 years ago by
Another benefit of making a file-per-package is that we could use them as requirements.txt files to pin versions if necessary. Or perhaps with directories as name/requirements.txt file.
comment:33 Changed 7 years ago by
Less fugly solution
sage --pip install --upgrade --force-reinstall <packagename>
comment:34 in reply to: ↑ 30 Changed 7 years ago by
Replying to jhpalmieri:
This might be a little ugly:
build/make/install
diff --git a/build/make/install b/build/make/install index 7469661..a164fc4 100755
a b while read PKG_NAME; do 473 473 474 474 # Add a target to remove the "installed" file for "sage -f" 475 475 echo >&5 "$PKG_NAME-clean:" 476 echo >&5 " sage --pip uninstall -y $PKG_NAME"476 echo >&5 " sage --pip list | grep $PKG_NAME && sage --pip uninstall -y $PKG_NAME || :" 477 477 echo >&5 478 478 done <"$SAGE_ROOT/build/pkgs/piprules" 479 479 Or the same but omitting
sage --pip list | grep $PKG_NAME &&
.
You need to omit it since pip list
does list BeautifulSoup
and our package name is beautifulsoup
(unless you grep without case sensitivity).
comment:35 Changed 7 years ago by
- Description modified (diff)
comment:36 in reply to: ↑ 29 Changed 7 years ago by
comment:37 Changed 7 years ago by
- Commit changed from fdba3ee0772b1885c3f60cb35f850fee576a7f9a to 571410a251d319ba0e4d904f9840892aea921c7e
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
571410a | Add rules for installing packages with pip
|
comment:38 Changed 7 years ago by
- Commit changed from 571410a251d319ba0e4d904f9840892aea921c7e to 52a3cf08a5c485ecc5b080ac19f7e5080ac4fc06
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
52a3cf0 | Add rules for installing packages with pip
|
comment:39 Changed 7 years ago by
- Dependencies #19119 deleted
comment:40 Changed 7 years ago by
- Reviewers changed from Vincent Delecroix to Vincent Delecroix, Volker Braun
- Status changed from needs_work to needs_review
comment:41 Changed 7 years ago by
This is good for me. Volker?
comment:42 Changed 7 years ago by
Whats the point of whitelisting only specific packages? Including trac, which has no mathematical relevance nor any utility to non-developers (and for those only barely). Having special entries in the Sage package list only makes sense if there is a need for it, e.g. because they are dependencies of non-pip (optional) packages.
IMHO if the user runs sage -f packagename
and packagename
is not a Sage package then either
- always try to
pip install packagename
, or - finish with
pip search packagename
and, if it finds something, tell the user that it can be installed viasage --pip
We can merge the current to appease the cargo cultists and finally get rid of some of the broken packages, but its not entirely satisfactory.
comment:43 Changed 7 years ago by
- Status changed from needs_review to positive_review
comment:44 Changed 7 years ago by
- Branch changed from u/jdemeyer/ticket/19187 to 52a3cf08a5c485ecc5b080ac19f7e5080ac4fc06
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Change sage -p to always install a package
Better help
Remove -f option to sage-spkg when running sage -i
Use PKG-clean target to implement sage -f PKG
Merge tag '6.9.beta6' into t/19119/ticket/19119
Add rules for installing packages with pip