Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Comply with PEP 518. #7309

Merged
merged 2 commits into from Jul 14, 2017
Merged

Comply with PEP 518. #7309

merged 2 commits into from Jul 14, 2017

Conversation

ghost
Copy link

@ghost ghost commented Apr 17, 2017

No description provided.

@rgommers rgommers self-requested a review April 17, 2017 20:20
@rgommers rgommers added Official binaries Items related to the official SciPy binaries, including wheels and vendored libs enhancement A new feature or improvement labels Apr 17, 2017
@rgommers
Copy link
Member

Thanks @xoviat, good idea. Will look at this in detail soon.

@rgommers rgommers added this to the 1.0 milestone Apr 17, 2017
@rgommers
Copy link
Member

This PR looks pretty straightforward, but I don't think we should merge it before support for PEP 518 is merged in pip (any day now, pypa/pip#4144) and we can test that things work as expected.

@rgommers
Copy link
Member

The PEP doesn't say anything about optional build dependencies (like Cython for building from a git repo), so I guess we'll just have to leave that out.

@rgommers
Copy link
Member

I don't think we should merge it before support for PEP 518 is merged in pip

It's merged now. Not in the latest pip release yet, but this PR can be tested with pip master.

@rgommers
Copy link
Member

Note that this can't be tested from a git checkout, because pip doesn't build wheels for pip install . currently: pypa/pip#4144 (comment). In the process of being fixed: pypa/pip#4562

@rgommers
Copy link
Member

Testing via sdist generation did yield an issue: include pyproject.toml should be added to MANIFEST.in.

@rgommers
Copy link
Member

Testing this turned up a fairly serious problem with PEP 518 (or maybe I'm misunderstanding something). The intention of the PEP is "Installation of the build system". This is further clarified like so:

... dependencies required to execute the build system (currently that means 
what dependencies are required to execute a setup.py file). 

For scipy that means that numpy should be in pyproject.toml (as done in this PR). That results in scipy being built against a numpy just downloaded from PyPI (or the cached equivalent) rather than the numpy already installed on the system. Building against the installed numpy would make more sense.

So requires=['numpy'] is definitely wrong, that causes crashes or import errors if the installed numpy has a lower C API number than the latest release.

Doing requires=['numpy==1.lowest.supported'] is better, but still feels a bit odd. For one, site.cfg can be located in the installed numpy right next to system_info.py, and of course that mechanism will break. Doc at https://github.com/numpy/numpy/blob/master/numpy/distutils/system_info.py#L71, file lookup at https://github.com/numpy/numpy/blob/master/numpy/distutils/system_info.py#L273

Even if we avoid requires=['numpy'] here, I'm sure lots of people will get that wrong in other packages.....

@njsmith @takluyver opinions?

@pv
Copy link
Member

pv commented Jun 29, 2017 via email

@pv
Copy link
Member

pv commented Jun 29, 2017 via email

@njsmith
Copy link

njsmith commented Jun 29, 2017

My expectation is that you want to use numpy == lowest-supported. I don't understand why that doesn't work -- isn't that what you want to build against? (I see your paragraph about that being "odd", but I don't know what any of it means :-).)

@pv
Copy link
Member

pv commented Jun 29, 2017 via email

@takluyver
Copy link
Contributor

Why does it disallow building against numpy already installed on the system?

In terms of the spec, it does not prevent that. But the implementation in pip will always install these requirements anew to do the build.

Might be worth bringing up on distutils-sig.

@pv
Copy link
Member

pv commented Jun 29, 2017 via email

@ghost
Copy link
Author

ghost commented Jun 29, 2017

Might be worth bringing up on distutils-sig.

This is not in line with the specification, but simply a pip implementation detail. If this is really a problem, then file a pip bug.

@njsmith
Copy link

njsmith commented Jun 29, 2017

That can't work reliably. Suppose that library A depends on numpy == 1.10.0 and on scipy, and someone does pip install A in a fresh virtualenv. There is no numpy installed, so your proposal would first scipy build against the latest numpy, and then that scipy + an old numpy would be installed, leaving the user with a broken environment.

Plus in general build-requirements can't be installed into the system environment for a number of reasons, and you'd really rather that builds use a clean environment for reproducibility and to reduce the number of cases that need testing, so using the system numpy is really not trivial. (IIRC @takluyver had to drop the feature of running builds in a real fresh environment for now due to some annoying technical problem, but I would hope pip adds it back in the future.)

Again, why do you consider pip's behavior suboptimal?

@pv
Copy link
Member

pv commented Jun 29, 2017

someone does pip install A in a fresh virtualenv

But this is not the case that we are discussing. This is about the situation where numpy is already installed in the virtualenv.

Moreover, also in the case that you describe, won't the current behavior of pip already produce broken environment? Is there any guarantee the numpy installed in the temporary environment is older release than what finally ends up being installed in the environment --- which is necessary to guarantee ABI compatibility?

@pv
Copy link
Member

pv commented Jun 29, 2017

The reasons why I believe pip's current behavior is suboptimal are explained in the pip issue above.
The discussion can be continued there, I think.

@njsmith
Copy link

njsmith commented Jun 29, 2017

But this is not the case that we are discussing. This is about the situation where numpy is already installed in the virtualenv.

Ok, but if your proposal to handle the case we're discussing will as a side effect break a case that we're not discussing, perhaps we should discuss both of them together :-)

Moreover, also in the case that you describe, won't the current behavior of pip already produce broken environment? Is there any guarantee the numpy installed in the temporary environment is older release than what finally ends up being installed in the environment?

That's why scipy should declare a build dependency on the oldest supported numpy. That's what gives you the guarantee that the resulting scipy binary will work in any environment it might be installed into.

@pv
Copy link
Member

pv commented Jun 29, 2017

Ok, so by "my proposal" you meant the >= in pyproject.toml --- not the point that if there's no need for pip to upgrade packages, it shoudn't. Your comment is relevant only for the former.

@njsmith
Copy link

njsmith commented Jun 29, 2017

No, because even if pip is modified to use system packages when possible, this will only happen if the system packages satisfy the declared version constraints. So you really want pip to use system packages if possible, AND some constraint that means "I'll accept any of these versions of they're already installed, but if none of them is installed give me the oldest". That's not what >= means though.

In any case, even if you extended pip's constraint language to support that, I gave an example of where it would still produce broken installs in a comment on the pip bug...

@pv
Copy link
Member

pv commented Jun 29, 2017

Yes, so the question is about >= vs == in the pyproject.toml. It's correct that for >= you'd need some extra markers in the generated wheel, at the minimum --- if you want things to behave more robustly than they do currently.

@rgommers
Copy link
Member

(I see your paragraph about that being "odd", but I don't know what any of it means :-).)

It's distutils, my technically correct description can't be clear almost by definition:)

Let's try in a handwaving way:

  • imagine you want to set up an environment with numpy, scipy & co buily against OpenBLAS or MKL

  • this likely requires a site.cfg with the correct paths to OpenBLAS or MKL

  • when you install with pip, there's two reasonable places to put that site.cfg:

    (1) ~/.numpy-site.cfg
    (2) site.cfg in installed_numpy/distutils/site.cfg

    of course if you want this to be specific to one virtualenv, then you only have option 2.

  • with the "put numpy in an isolated build env" as the current implementation of PEP 518 in pip, option 2 cannot work.

@pv
Copy link
Member

pv commented Jun 30, 2017 via email

@rgommers
Copy link
Member

The site.cfg stuff doesn't really work with pip.

Why not? pip simply calls setup.py install --some-more-args, so it should work fine. The only thing you can't do is put it next to setup.py, because that's not possible for an sdist download. But that's option 3; options 1 and 2 should work fine.

@takluyver
Copy link
Contributor

pip simply calls setup.py install --some-more-args

This is gradually becoming less accurate: in an increasing number of situations, pip will call setup.py bdist_wheel and then cache & install the resulting wheel. Pip 10 is likely to extend this to installing from a local directory. I'm not sure if this makes a difference to the discussion here, but I thought I'd mention it.

@rgommers
Copy link
Member

I'm not sure if this makes a difference to the discussion here, but I thought I'd mention it.

You're completely right. I was sloppy with the details; it doesn't make any difference though. As long as a normal build is done which invokes numpy.distutils then it doesn't really matter if you're building a wheel or doing a direct install, and whether it's pip or python setup.py xxx. The site.cfg mechanism always works the same.

@rgommers
Copy link
Member

The right version of this should be:

[build-system]
requires = [
    "wheel",
    "setuptools",
    "numpy==1.8.2; python_version<=3.4",
    "numpy==1.9.3; python_version==3.5",
    "numpy==1.12.1; python_version==3.6",
    "numpy==1.13.1; python_version>=3.7",
]

@xoviat can you update your PR?

We're going to just have to ignore the site.cfg issue, it can still be made to work with ~.numpy-site.cfg so it will only affect a very small group of users (and it's not fixable).

@ghost
Copy link
Author

ghost commented Jul 13, 2017

@rgommers Done. It's better to get this stuff hashed out sooner rather than later.

@rgommers rgommers merged commit d83699f into scipy:master Jul 14, 2017
@rgommers
Copy link
Member

Merged, thanks @xoviat, all!

@TomAugspurger
Copy link
Contributor

Forgive the uninformed question, but I'm trying to add a pyproject.toml for pandas, and was basing it off Scipy's.

I've cloned scipy, and setup a clean virtual environment with

Package    Version             Location                             
---------- ------------------- -------------------------------------
Cython     0.26                
pip        10.0.0.dev0         /Users/taugspurger/sandbox/pip       
setuptools 36.2.7.post20170824 /Users/taugspurger/sandbox/setuptools
wheel      0.29.0              

pip and setuptools are master as of today.

I run pip wheel . from within the scipy repository and get an exception related to the python_version<=3.4 in the pyproject.toml:

 $ pip wheel .
Processing /Users/taugspurger/sandbox/scipy
Collecting numpy>=1.8.2 (from scipy==1.0.0.dev0+84fd3a5)
  File was already downloaded /Users/taugspurger/sandbox/scipy/numpy-1.13.1-cp36-cp36m-macosx_10_6_intel.macosx_10_9_intel.macosx_10_9_x86_64.macosx_10_10_intel.macosx_10_10_x86_64.whl
Skipping numpy, due to already being wheel.
Building wheels for collected packages: scipy
Exception:
Traceback (most recent call last):
  File "/Users/taugspurger/sandbox/pip/pip/_vendor/packaging/markers.py", line 276, in __init__
    self._markers = _coerce_parse_result(MARKER.parseString(marker))
  File "/Users/taugspurger/sandbox/pip/pip/_vendor/pyparsing.py", line 1632, in parseString
    raise exc
  File "/Users/taugspurger/sandbox/pip/pip/_vendor/pyparsing.py", line 1622, in parseString
    loc, tokens = self._parse( instring, 0 )
  File "/Users/taugspurger/sandbox/pip/pip/_vendor/pyparsing.py", line 1379, in _parseNoCache
    loc,tokens = self.parseImpl( instring, preloc, doActions )
  File "/Users/taugspurger/sandbox/pip/pip/_vendor/pyparsing.py", line 3395, in parseImpl
    loc, exprtokens = e._parse( instring, loc, doActions )
  File "/Users/taugspurger/sandbox/pip/pip/_vendor/pyparsing.py", line 1379, in _parseNoCache
    loc,tokens = self.parseImpl( instring, preloc, doActions )
  File "/Users/taugspurger/sandbox/pip/pip/_vendor/pyparsing.py", line 3717, in parseImpl
    return self.expr._parse( instring, loc, doActions, callPreParse=False )
  File "/Users/taugspurger/sandbox/pip/pip/_vendor/pyparsing.py", line 1379, in _parseNoCache
    loc,tokens = self.parseImpl( instring, preloc, doActions )
  File "/Users/taugspurger/sandbox/pip/pip/_vendor/pyparsing.py", line 3378, in parseImpl
    loc, resultlist = self.exprs[0]._parse( instring, loc, doActions, callPreParse=False )
  File "/Users/taugspurger/sandbox/pip/pip/_vendor/pyparsing.py", line 1379, in _parseNoCache
    loc,tokens = self.parseImpl( instring, preloc, doActions )
  File "/Users/taugspurger/sandbox/pip/pip/_vendor/pyparsing.py", line 3545, in parseImpl
    raise maxException
  File "/Users/taugspurger/sandbox/pip/pip/_vendor/pyparsing.py", line 3530, in parseImpl
    ret = e._parse( instring, loc, doActions )
  File "/Users/taugspurger/sandbox/pip/pip/_vendor/pyparsing.py", line 1379, in _parseNoCache
    loc,tokens = self.parseImpl( instring, preloc, doActions )
  File "/Users/taugspurger/sandbox/pip/pip/_vendor/pyparsing.py", line 3717, in parseImpl
    return self.expr._parse( instring, loc, doActions, callPreParse=False )
  File "/Users/taugspurger/sandbox/pip/pip/_vendor/pyparsing.py", line 1379, in _parseNoCache
    loc,tokens = self.parseImpl( instring, preloc, doActions )
  File "/Users/taugspurger/sandbox/pip/pip/_vendor/pyparsing.py", line 3395, in parseImpl
    loc, exprtokens = e._parse( instring, loc, doActions )
  File "/Users/taugspurger/sandbox/pip/pip/_vendor/pyparsing.py", line 1379, in _parseNoCache
    loc,tokens = self.parseImpl( instring, preloc, doActions )
  File "/Users/taugspurger/sandbox/pip/pip/_vendor/pyparsing.py", line 3545, in parseImpl
    raise maxException
  File "/Users/taugspurger/sandbox/pip/pip/_vendor/pyparsing.py", line 3530, in parseImpl
    ret = e._parse( instring, loc, doActions )
  File "/Users/taugspurger/sandbox/pip/pip/_vendor/pyparsing.py", line 1379, in _parseNoCache
    loc,tokens = self.parseImpl( instring, preloc, doActions )
  File "/Users/taugspurger/sandbox/pip/pip/_vendor/pyparsing.py", line 3545, in parseImpl
    raise maxException
  File "/Users/taugspurger/sandbox/pip/pip/_vendor/pyparsing.py", line 3530, in parseImpl
    ret = e._parse( instring, loc, doActions )
  File "/Users/taugspurger/sandbox/pip/pip/_vendor/pyparsing.py", line 1383, in _parseNoCache
    loc,tokens = self.parseImpl( instring, preloc, doActions )
  File "/Users/taugspurger/sandbox/pip/pip/_vendor/pyparsing.py", line 2413, in parseImpl
    raise ParseException(instring, loc, self.errmsg, self)
pip._vendor.pyparsing.ParseException: Expected {Group:({{{"implementation_version" | "platform_python_implementation" | "implementation_name" | "python_full_version" | "platform_release" | "platform_version" | "platform_machine" | "platform_system" | "python_version" | "sys_platform" | "os_name" | "os.name" | "sys.platform" | "platform.version" | "platform.machine" | "platform.python_implementation" | "python_implementation" | "extra"} | {quoted string, starting with ' ending with ' | quoted string, starting with " ending with "}} {"===" | "==" | ">=" | "<=" | "!=" | "~=" | ">" | "<" | "not in" | "in"} {{"implementation_version" | "platform_python_implementation" | "implementation_name" | "python_full_version" | "platform_release" | "platform_version" | "platform_machine" | "platform_system" | "python_version" | "sys_platform" | "os_name" | "os.name" | "sys.platform" | "platform.version" | "platform.machine" | "platform.python_implementation" | "python_implementation" | "extra"} | {quoted string, starting with ' ending with ' | quoted string, starting with " ending with "}}}) | Group:({Suppress:("(") Forward: ... Suppress:(")")})} (at char 16), (line:1, col:17)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/taugspurger/sandbox/pip/pip/basecommand.py", line 214, in main
    status = self.run(options, args)
  File "/Users/taugspurger/sandbox/pip/pip/commands/wheel.py", line 188, in run
    wheels_built_successfully = wb.build(session=session)
  File "/Users/taugspurger/sandbox/pip/pip/wheel.py", line 793, in build
    python_tag=python_tag,
  File "/Users/taugspurger/sandbox/pip/pip/wheel.py", line 648, in _build_one
    self._install_build_reqs(build_reqs, prefix)
  File "/Users/taugspurger/sandbox/pip/pip/wheel.py", line 628, in _install_build_reqs
    for r in reqs]
  File "/Users/taugspurger/sandbox/pip/pip/wheel.py", line 628, in <listcomp>
    for r in reqs]
  File "/Users/taugspurger/sandbox/pip/pip/req/req_install.py", line 184, in from_line
    markers = Marker(markers)
  File "/Users/taugspurger/sandbox/pip/pip/_vendor/packaging/markers.py", line 280, in __init__
    raise InvalidMarker(err_str)
pip._vendor.packaging.markers.InvalidMarker: Invalid marker: 'python_version<=3.4', parse error at '3.4'

Does that seem like a bug in setuptools / pip, or is it a problem with scipy's pyproject.toml? (Or, more likely, am I doing something wrong?)

@takluyver
Copy link
Contributor

I think 3.4 needs to be quoted.

@ghost
Copy link
Author

ghost commented Aug 24, 2017

FYI PEP 518 is not implemented in pip.

@pv
Copy link
Member

pv commented Aug 24, 2017

@xoviat: pypa/pip#4144

@TomAugspurger
Copy link
Contributor

TomAugspurger commented Aug 24, 2017

Yeah, and I'm using pip master. Quoting 3.4 lead to a different error. I'll keep debugging this and report back (hopefully with a PR). Thanks!

@ghost
Copy link
Author

ghost commented Aug 24, 2017 via email

@pv
Copy link
Member

pv commented Aug 24, 2017

But I guess we should add the double quotes (or single quotes) in the environment markers, as they seem to be required by PEP 508: gh-7792

@ghost
Copy link
Author

ghost commented Aug 24, 2017

Also, should we add cython?

@pv
Copy link
Member

pv commented Aug 24, 2017

Yep.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement Official binaries Items related to the official SciPy binaries, including wheels and vendored libs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants