Opened 5 years ago

Closed 5 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) Commit: 52a3cf08a5c485ecc5b080ac19f7e5080ac4fc06
Dependencies: Stopgaps:

Description (last modified by vdelecroix)

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

follow-up: #19213, #19220

Change History (44)

comment:1 Changed 5 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/19187

comment:2 Changed 5 years ago by jdemeyer

  • Commit set to adef308eaf6a6c478301471c3c2350260b39a5fe
  • Status changed from new to needs_review

New commits:

93ec09bChange sage -p to always install a package
c6ddd0eBetter help
f3d7ef3Remove -f option to sage-spkg when running sage -i
752e269Use PKG-clean target to implement sage -f PKG
1812e4fMerge tag '6.9.beta6' into t/19119/ticket/19119
adef308Add rules for installing packages with pip

comment:3 Changed 5 years ago by jdemeyer

  • Priority changed from major to critical

comment:4 follow-up: Changed 5 years ago by kcrisman

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 5 years ago by jdemeyer

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 5 years ago by kcrisman

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 5 years ago by git

  • Commit changed from adef308eaf6a6c478301471c3c2350260b39a5fe to fdba3ee0772b1885c3f60cb35f850fee576a7f9a

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

fdba3eeAdd mercurial and sqlalchemy (ex-standard packages)

comment:8 follow-up: Changed 5 years ago by kcrisman

(You know, this could be very helpful for disentangling the many packages in sagenb.)

comment:9 in reply to: ↑ 8 Changed 5 years ago by jdemeyer

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: Changed 5 years ago by vdelecroix

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: Changed 5 years ago by tscrim

Could this also be modified to work for nzmath and pyx?

comment:12 in reply to: ↑ 10 Changed 5 years ago by kcrisman

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: Changed 5 years ago by jdemeyer

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 5 years ago by jdemeyer

Replying to tscrim:

Could this also be modified to work for nzmath and pyx?

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 5 years ago by tscrim

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 5 years ago by vdelecroix

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

Last edited 5 years ago by vdelecroix (previous) (diff)

comment:17 in reply to: ↑ 13 Changed 5 years ago by vdelecroix

  • 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 or sage --pip install PKG? With my patch here, the user just needs to know sage -i PKG.

No. And as Karl-Dieter said it would be good to add tests for them # optional - biopython.

Vincent

comment:18 follow-up: Changed 5 years ago by 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".

comment:19 follow-up: Changed 5 years ago by 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.

comment:20 Changed 5 years ago by jhpalmieri

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  
    55
    66from __future__ import print_function
    77
    8 import os, sys, argparse, re
     8import os, argparse, re, subprocess
    99from string import join
    1010
    1111if "SAGE_ROOT" not in os.environ:
    if args['category'] == 'installed': 
    9898# Any other categories
    9999else:
    100100    if args['version']:
     101        try:
     102            pip_output = subprocess.check_output(['pip', 'list'], stderr=subprocess.STDOUT)
     103        except CalledProcessError:
     104            pip_output = ''
     105
    101106        # Display the columns' name
    102107        if not args['dump']:
    103108            print("{:.<40} {} ({})\n".format("[package] ", "[latest version]",
    104109                                             "[installed version]"))
    105110        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'
    106119            line = [p, local.get(p, remote.get(p, 'not_found')),
    107                     installed.get(p, 'not_installed')]
     120                    installed_version]
    108121            if args['dump']:
    109122                print(join(line, ' '))
    110123            else:

(This is not a complete patch, just an idea.)

comment:21 Changed 5 years ago by jhpalmieri

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 5 years ago by jhpalmieri

See #19213.

comment:23 in reply to: ↑ 19 Changed 5 years ago by vdelecroix

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 5 years ago by vdelecroix

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: Changed 5 years ago by 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?

comment:26 in reply to: ↑ 25 Changed 5 years ago by jdemeyer

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: Changed 5 years ago by vdelecroix

  • 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 5 years ago by jdemeyer

Replying to vdelecroix:

Should we explicit ask on sage-devel for removing them from the server?

See #19220.

comment:29 follow-ups: Changed 5 years ago by vbraun

  • 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: Changed 5 years ago by 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 
    473473
    474474    # Add a target to remove the "installed" file for "sage -f"
    475475    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 || :"
    477477    echo >&5
    478478done <"$SAGE_ROOT/build/pkgs/piprules"
    479479

Or the same but omitting sage --pip list | grep $PKG_NAME &&.

comment:31 in reply to: ↑ 29 Changed 5 years ago by jhpalmieri

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 5 years ago by vbraun

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 5 years ago by vbraun

Less fugly solution

sage --pip install --upgrade --force-reinstall <packagename>

comment:34 in reply to: ↑ 30 Changed 5 years ago by vdelecroix

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 
    473473
    474474    # Add a target to remove the "installed" file for "sage -f"
    475475    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 || :"
    477477    echo >&5
    478478done <"$SAGE_ROOT/build/pkgs/piprules"
    479479

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 5 years ago by vdelecroix

  • Description modified (diff)

comment:36 in reply to: ↑ 29 Changed 5 years ago by jdemeyer

Replying to vbraun:

Also breaks the buildbot script that assumes that build/pkgs/* has a corresponding log file

Didn't you agree in #18993 that ordinary files in build/pkgs should be ignored?

comment:37 Changed 5 years ago by git

  • Commit changed from fdba3ee0772b1885c3f60cb35f850fee576a7f9a to 571410a251d319ba0e4d904f9840892aea921c7e

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

571410aAdd rules for installing packages with pip

comment:38 Changed 5 years ago by git

  • Commit changed from 571410a251d319ba0e4d904f9840892aea921c7e to 52a3cf08a5c485ecc5b080ac19f7e5080ac4fc06

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

52a3cf0Add rules for installing packages with pip

comment:39 Changed 5 years ago by jdemeyer

  • Dependencies #19119 deleted

comment:40 Changed 5 years ago by jdemeyer

  • Reviewers changed from Vincent Delecroix to Vincent Delecroix, Volker Braun
  • Status changed from needs_work to needs_review

comment:41 Changed 5 years ago by vdelecroix

This is good for me. Volker?

comment:42 Changed 5 years ago by vbraun

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 via sage --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 5 years ago by vbraun

  • Status changed from needs_review to positive_review

comment:44 Changed 5 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/19187 to 52a3cf08a5c485ecc5b080ac19f7e5080ac4fc06
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.