Opened 5 months ago

Closed 3 months ago

#31180 closed enhancement (fixed)

Add snappy as a pip package

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.3
Component: packages: optional Keywords:
Cc: dunfield, culler, dimpase, vdelecroix, slelievre Merged in:
Authors: Matthias Koeppe Reviewers: Nathan Dunfield
Report Upstream: Reported upstream. No feedback yet. Work issues:
Branch: fb5366d (Commits, GitHub, GitLab) Commit: fb5366d691b94f97d3ca5afc2673142ee91b5584
Dependencies: #31132 Stopgaps:

Status badges

Change History (35)

comment:1 Changed 5 months ago by mkoeppe

  • Branch set to u/mkoeppe/add_snappy_as_a_pip_package

comment:2 Changed 5 months ago by git

  • Commit set to 46f7b0e46732c647d2889486117e281e2713c3cb

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

46f7b0ebuild/pkgs/snappy: New pip package

comment:3 Changed 5 months ago by mkoeppe

  • Authors set to Matthias Koeppe

New commits:

46f7b0ebuild/pkgs/snappy: New pip package

comment:4 Changed 5 months ago by mkoeppe

Unfortunately --no-deps is not valid syntax in requirements.txt files

comment:5 Changed 5 months ago by dunfield

The full list of outside dependencies for snappy are:

pypng
decorator
future
ipython
cypari|cypari2

so just using snappy spherogram plink FXrays pypng should do the trick (note I forgot about pypng in my post on #31176). But maybe include decorator and future just in case those aren't part of some future version of Sage.

comment:6 Changed 5 months ago by mkoeppe

  • Description modified (diff)

comment:7 Changed 5 months ago by dunfield

Hmm, I see what you mean by --no-deps. I guess one can only give that flag to pip but not embed it in requirements.txt.

comment:8 Changed 5 months ago by mkoeppe

Looking at snappy's setup.py, I see that you are already trying to conditionalize away the issue with cypari.

 install_requires = ['plink>=2.3.1', 'spherogram>=1.8.3', 'FXrays>=1.3',
                    'pypng', 'decorator', 'future', 'snappy_manifolds>=1.1.1']
try:
    import sage
except ImportError:
    install_requires.append('cypari>=2.2')
    install_requires.append('ipython>=0.13')
    if sys.version_info < (3, 4):
        install_requires.append('ipython<6.0')
    if sys.platform == 'win32':
        install_requires.append('pyreadline>=2.0')

Unfortunately I think the presence of wheels for snappy on PyPI will circumvent it.

comment:9 Changed 5 months ago by mkoeppe

Would making the cypari dependency an extras_require in snappy be an acceptable way forward for you?

comment:10 Changed 5 months ago by dunfield

Yeah, with binary wheels the install_requires are baked in and our tests in setup.py are of no help.

I don't think extras_require helps here as cypari* is a core dependency for snappy and so is in no sense optional.

comment:11 Changed 5 months ago by dunfield

It's inelegant, but I suspect that, in light of pip's limitations, the best thing is simply to make requirements.txt be the single line snappy. If wheels are available, this will result in a completely unused cypari package, but this will not interfere with anything and just wastes 30M (on linux, unpacked), which is small in context snappy (165 M), much less Sage itself (2G+). (If instead pip ends up building from source, then there is no issue at all because of our setup.py.)

This also has the advantage it makes it less likely that this Sage pip package will need updating.

This argument is especially compelling if we throw in the optional database snappy_15_knots into this Sage pip package, as that is another 110M uncompressed.

comment:12 follow-up: Changed 5 months ago by mkoeppe

I am not sure. It does not seem like a very robust solution.

Just as a data point: I seem to be having trouble installing cypari into Sage here on macOS using python 3.9, for which no prebuilt wheel is available.

Building wheels for collected packages: cypari
  Created temporary directory: /private/var/folders/38/wnh4gf1552g_crsjnv2vmmww0000gp/T/pip-wheel-8xuqmqye
  Building wheel for cypari (setup.py) ...   Destination directory: /private/var/folders/38/wnh4gf1552g_crsjnv2vmmww0000gp/T/pip-wheel-8xuqmqye
  Running command /Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/local/bin/python3 -u -c 'import sys, setuptools, tokenize; sys.argv[0] = '"'"'/private/var/folders/38/wnh4gf1552g_crsjnv2vmmww0000gp/T/pip-install-sbgfgcz3/cypari/setup.py'"'"'; __file__='"'"'/private/var/folders/38/wnh4gf1552g_crsjnv2vmmww0000gp/T/pip-install-sbgfgcz3/cypari/setup.py'"'"';f=getattr(tokenize, '"'"'open'"'"', open)(__file__);code=f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' bdist_wheel -d /private/var/folders/38/wnh4gf1552g_crsjnv2vmmww0000gp/T/pip-wheel-8xuqmqye
  running bdist_wheel
  running build
  running build_py
  creating build
  creating build/lib.macosx-10.15-x86_64-3.9
  creating build/lib.macosx-10.15-x86_64-3.9/cypari
  copying cypari/version.py -> build/lib.macosx-10.15-x86_64-3.9/cypari
  copying cypari/memory.py -> build/lib.macosx-10.15-x86_64-3.9/cypari
  copying cypari/__init__.py -> build/lib.macosx-10.15-x86_64-3.9/cypari
  copying cypari/test.py -> build/lib.macosx-10.15-x86_64-3.9/cypari
  copying cypari/tests.py -> build/lib.macosx-10.15-x86_64-3.9/cypari
  copying cypari/py3tests.py -> build/lib.macosx-10.15-x86_64-3.9/cypari
  copying cypari/py2tests.py -> build/lib.macosx-10.15-x86_64-3.9/cypari
  running build_ext
  error: [Errno 2] No such file or directory: 'cypari/_pari_py3.c' -> 'cypari/_pari.c'
error
  ERROR: Failed building wheel for cypari
...
Installing collected packages: cypari
  Created temporary directory: /private/var/folders/38/wnh4gf1552g_crsjnv2vmmww0000gp/T/pip-record-5v39xxkr
    Running command /Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/local/bin/python3 -u -c 'import sys, setuptools, tokenize; sys.argv[0] = '"'"'/private/var/folders/38/wnh4gf1552g_crsjnv2vmmww0000gp/T/pip-install-sbgfgcz3/cypari/setup.py'"'"'; __file__='"'"'/private/var/folders/38/wnh4gf1552g_crsjnv2vmmww0000gp/T/pip-install-sbgfgcz3/cypari/setup.py'"'"';f=getattr(tokenize, '"'"'open'"'"', open)(__file__);code=f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' install --record /private/var/folders/38/wnh4gf1552g_crsjnv2vmmww0000gp/T/pip-record-5v39xxkr/install-record.txt --single-version-externally-managed --compile --install-headers /Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/local/include/site/python3.9/cypari
    running install
    running build
    running build_py
    running build_ext
    Building gmp ...
    rm -f gp-dyn
    /Library/Developer/CommandLineTools/usr/bin/ld  -o gp-dyn -L"/private/var/folders/38/wnh4gf1552g_crsjnv2vmmww0000gp/T/pip-install-sbgfgcz3/cypari/build/pari_src/Odarwin-x86_64" -search_paths_first -L/Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/local/lib -Wl,-rpath,/Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/local/lib  emacs.o gp.o gp_rl.o texmacs.o whatnow.o plotX.o  -lreadline -lpari -L/usr/X11R6/lib -lX11
    ld: unknown option: -Wl,-rpath,/Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/local/lib
    make: *** [gp-dyn] Error 1
    ***Failed to build PARI library***
    Running setup.py install for cypari ... error
ERROR: Command errored out with exit status 1: /Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/local/bin/python3 -u -c 'import sys, setuptools, tokenize; sys.argv[0] = '"'"'/private/var/folders/38/wnh4gf1552g_crsjnv2vmmww0000gp/T/pip-install-sbgfgcz3/cypari/setup.py'"'"'; __file__='"'"'/private/var/folders/38/wnh4gf1552g_crsjnv2vmmww0000gp/T/pip-install-sbgfgcz3/cypari/setup.py'"'"';f=getattr(tokenize, '"'"'open'"'"', open)(__file__);code=f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' install --record /private/var/folders/38/wnh4gf1552g_crsjnv2vmmww0000gp/T/pip-record-5v39xxkr/install-record.txt --single-version-externally-managed --compile --install-headers /Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/local/include/site/python3.9/cypari Check the logs for full command output.
Exception information:
Traceback (most recent call last):
  File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/local/lib/python3.9/site-packages/pip/_internal/req/req_install.py", line 838, in install
    success = install_legacy(
  File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/local/lib/python3.9/site-packages/pip/_internal/operations/install/legacy.py", line 86, in install
    raise LegacyInstallFailure
pip._internal.operations.install.legacy.LegacyInstallFailure

comment:13 follow-up: Changed 5 months ago by mkoeppe

Also ./sage -pip install -v -v snappy on the same system gives:

  ...
  gcc -Wno-unused-result -Wsign-compare -Wunreachable-code -fno-common -dynamic -DNDEBUG -g -fwrapv -O3 -Wall -I/usr/local/include -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk -I/Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk/usr/include -I/Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk/System/Library/Frameworks/Tk.framework/Versions/8.5/Headers -Ikernel/headers -Ikernel/unix_kit -Ikernel/addl_code -Ikernel/real_type -I/usr/local/include -I/usr/local/opt/openssl@1.1/include -I/usr/local/opt/sqlite/include -I/Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/local/include -I/usr/local/Cellar/python@3.9/3.9.1_1/Frameworks/Python.framework/Versions/3.9/include/python3.9 -c kernel/kernel_code/Dirichlet.c -o build/temp.macosx-10.15-x86_64-3.9/kernel/kernel_code/Dirichlet.o
  kernel/kernel_code/Dirichlet.c:83:10: warning: non-portable path to file '"dirichlet.h"'; specified path differs in case from file name on disk [-Wnonportable-include-path]
  #include "Dirichlet.h"
           ^~~~~~~~~~~~~
           "dirichlet.h"
  kernel/kernel_code/Dirichlet.c:118:91: error: unknown type name 'MatrixPairList'; did you mean 'MatrixParity'?
  static void         array_to_matrix_pair_list(O31Matrix generators[], int num_generators, MatrixPairList *gen_list);
                                                                                            ^~~~~~~~~~~~~~
                                                                                            MatrixParity
  kernel/headers/SnapPea.h:100:3: note: 'MatrixParity' declared here
  } MatrixParity;
    ^
  kernel/kernel_code/Dirichlet.c:119:52: error: unknown type name 'MatrixPairList'; did you mean 'MatrixParity'?
  static Boolean      is_matrix_on_list(O31Matrix m, MatrixPairList *gen_list);
                                                     ^~~~~~~~~~~~~~
                                                     MatrixParity
  kernel/headers/SnapPea.h:100:3: note: 'MatrixParity' declared here
  } MatrixParity;
    ^
  kernel/kernel_code/Dirichlet.c:120:56: error: unknown type name 'MatrixPairList'; did you mean 'MatrixParity'?
  static void         insert_matrix_on_list(O31Matrix m, MatrixPairList *gen_list);
                                                         ^~~~~~~~~~~~~~
                                                         MatrixParity

comment:14 in reply to: ↑ 12 Changed 5 months ago by dunfield

Replying to mkoeppe:

I am not sure. It does not seem like a very robust solution.

Not that it means much, but we haven't had any reports of problems with sage -pip install snappy as our default install instructions.

I agree it would be preferable just to pass the --no-deps flag to pip, but my understanding is that this is not possible in the context of a "sage pip package"?

Just as a data point: I seem to be having trouble installing cypari into Sage here on macOS using python 3.9, for which no prebuilt wheel is available.

Ouch, that's a bug on our part, it seems we broke the sdist tarball in 2.4.0, on all platforms, no less: we moved some files around and forgot to update MANIFEST.in. We'll release 2.4.1 to fix this shortly.

comment:15 in reply to: ↑ 13 ; follow-up: Changed 5 months ago by dunfield

Replying to mkoeppe:

Also ./sage -pip install -v -v snappy on the same system gives:

Marc Culler points out the likely cause of this: there is a dirichlet.h in $SAGELOCAL/include that is part of arb as well as a Dirichlet.h that's part of SnapPy. Since macOS file system default to case-insensitive, there's a name collision which triggers the nonportable-include-path warning.

comment:16 in reply to: ↑ 15 ; follow-up: Changed 5 months ago by mkoeppe

Replying to dunfield:

Replying to mkoeppe:

Also ./sage -pip install -v -v snappy on the same system gives:

Marc Culler points out the likely cause of this: there is a dirichlet.h in $SAGELOCAL/include that is part of arb as well as a Dirichlet.h that's part of SnapPy. Since macOS file system default to case-insensitive, there's a name collision which triggers the nonportable-include-path warning.

I think it's actually coming from /usr/local/include/ on my system (from homebrew's arb).

The question is where the first -I/usr/local/include on the gcc command line is coming from. This is causing the wrong file to be picked up. (The rest of the order of the includes looks fine.)

comment:17 Changed 5 months ago by git

  • Commit changed from 46f7b0e46732c647d2889486117e281e2713c3cb to 5a76fdc85fce746601f3893ca2ab54cdb7604e18

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

5a76fdcbuild/pkgs/snappy: Update dependencies, requirements.txt, add comments

comment:18 Changed 5 months ago by git

  • Commit changed from 5a76fdc85fce746601f3893ca2ab54cdb7604e18 to f2ca439b97ba78c01e22305ac769e13ba819b463

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

f2ca439No future

comment:19 Changed 5 months ago by git

  • Commit changed from f2ca439b97ba78c01e22305ac769e13ba819b463 to c8583bb376165f2c96751ed6a4e2eea046aa17de

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

c8583bbbuild/pkgs/snappy/requirements.txt: Avoid cypari 2.4.0

comment:20 in reply to: ↑ 16 ; follow-up: Changed 5 months ago by dunfield

Replying to mkoeppe:

The question is where the first -I/usr/local/include on the gcc command line is coming from. This is causing the wrong file to be picked up. (The rest of the order of the includes looks fine.)

On my macOS 10.15 box, with Python in /Frameworks from Python.org, I get:

gcc -Wno-unused-result -Wsign-compare -Wunreachable-code -fno-common -dynamic -DNDEBUG -g -fwrapv -O3 -Wall -arch x86_64 -g 
   -Ikernel/headers -Ikernel/unix_kit -Ikernel/addl_code -Ikernel/real_type 
   -I/Library/Frameworks/Python.framework/Versions/3.9/include/python3.9 -c 
   kernel/addl_code/get_gluing_equations.c -o build/temp.macosx-10.9-x86_64-3.9/kernel/addl_code/get_gluing_equations.o

In particular, the includes start with the SnapPy specific ones and then the location of Python.h as desired. Maybe your first -I /usr/local/include is somehow coming from CFLAGS, or from the CFLAGS that were set when Python itself was built? I believe setuptools uses as least some of the latter.

comment:21 in reply to: ↑ 20 Changed 5 months ago by dunfield

Replying to dunfield:

Maybe your first -I /usr/local/include is somehow coming from CFLAGS, or from the CFLAGS that were set when Python itself was built? I believe setuptools uses as least some of the latter.

Assuming you're using homebrew's Python, the above seems quite likely as there I get:

$ /usr/local/Cellar/python@3.9/3.9.0_1/bin/python3
>>> import sysconfig
>>> sysconfig.get_config_var('CFLAGS')
'-Wno-unused-result -Wsign-compare -Wunreachable-code -fno-common -dynamic -DNDEBUG -g -fwrapv -O3 -Wall 
-isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk -I/Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk/usr/include 
-I/Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk/System/Library/Frameworks/Tk.framework/Versions/8.5/Headers'

comment:22 Changed 5 months ago by mkoeppe

Yes, my python3 is (Sage's venv over) homebrew python3.9 -- so this (the CFLAGS from sysconfig, not the environment) is how the -I/usr/local/include is entering on my system.

This is rather unfortunate and may also be the cause of the issues reported in #31132.

comment:23 Changed 5 months ago by dimpase

I have opened https://github.com/3-manifolds/SnapPy/issues/21 and commented on https://github.com/fredrik-johansson/arb/issues/239 - where the issue of arb header names was already brought up as causing problems.

comment:24 Changed 5 months ago by dimpase

  • Report Upstream changed from N/A to Reported upstream. No feedback yet.

comment:25 Changed 5 months ago by mkoeppe

  • Dependencies set to #31132

comment:26 Changed 5 months ago by git

  • Commit changed from c8583bb376165f2c96751ed6a4e2eea046aa17de to fb5366d691b94f97d3ca5afc2673142ee91b5584

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

2c60025m4/sage_check_python_for_venv.m4: Warn if python3 is misconfigured like it is currently in homebrew
ee679bdbuild/pkgs/python3/spkg-configure.m4: Do not check sysconfig CPPFLAGS so that /usr/bin/python3 is accepted on macOS; reject broken homebrew python3 unless requested explicitly with configgure --with-python=...
fb5366dMerge branch 't/31132/homebrew__unused_packages__singular__pari_______in__usr_local_leak_into_build_when_using_homebrew_s_python3' into t/31180/add_snappy_as_a_pip_package

comment:27 Changed 5 months ago by mkoeppe

  • Status changed from new to needs_review

With the fix from #31132 (rejecting broken homebrew python3) and rejecting the broken cypari 2.4.0 via requirements.txt, this now seems to work well here on macOS.

comment:28 Changed 5 months ago by mkoeppe

  • Reviewers set to https://github.com/mkoeppe/sage/actions/runs/473272399, https://github.com/mkoeppe/sage/actions/runs/473272391, ...

comment:29 Changed 5 months ago by mkoeppe

  • Cc vdelecroix added

comment:30 Changed 5 months ago by mkoeppe

Waiting for review

comment:31 Changed 5 months ago by dunfield

  • Status changed from needs_review to positive_review

Tried it out, sage -i snappy installs working snappy as expected, setting positive review.

comment:32 Changed 5 months ago by vdelecroix

I am very glad that we will have better support for snappy in Sage!

comment:33 Changed 5 months ago by mkoeppe

  • Reviewers changed from https://github.com/mkoeppe/sage/actions/runs/473272399, https://github.com/mkoeppe/sage/actions/runs/473272391, ... to Nathan Dunfield

comment:34 Changed 3 months ago by slelievre

  • Cc slelievre added

This probably makes #20739 obsolete.

comment:35 Changed 3 months ago by vbraun

  • Branch changed from u/mkoeppe/add_snappy_as_a_pip_package to fb5366d691b94f97d3ca5afc2673142ee91b5584
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.