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

Updated some extra requirements #797

Closed
wants to merge 2 commits into from
Closed

Conversation

a-recknagel
Copy link
Contributor

@a-recknagel a-recknagel commented Nov 18, 2019

And also added setuptools explicitly as build backend. phantomjs is no longer on pyPI and didn't seem to be necessary for generating the docs, and vtk doesn't have a python3.7 compatible wheel for version 8.1.1.

Environment:

  • os: Ubuntu 18.04.3
  • python: 3.7.2+
  • nodejs: v8.10.0
  • npm: 3.5.2
  • phantomjs: 2.1.1

Tests run:

  • install
    • pip install -e . ✔️
    • pip install -e .[tests] ✔️
    • pip install -e .[extras] ✔️
    • pip install -e .[doc] ✔️
  • build
    • pip wheel -w dist . --no-deps ✔️
    • nbsite generate-rst --org pyviz --project-name panel && nbsite build --what=html --output=builtdocs ✔️
  • tests
    • pytest panel/tests/ ✔️
    • manual execution of all cells of getting_started/Introduction.ipynb ✔️

@ceball
Copy link
Member

ceball commented Nov 18, 2019

It's great you did this testing, thanks.

added setuptools explicitly as build backend

That seems like a good idea to me - thanks for adding it. My understanding (out of date, topped up just now by skimming pypa/setuptools#1698) is that for it to be effective, either we'd also need to require a fairly recent setuptools (newer than the 30.3 we currently state, anyway) or we'd also need to include pyproject.toml in MANIFEST.in. Any idea about that, or practical experience? I don't have any.

vtk doesn't have a python3.7 compatible wheel for version 8.1.1

There's unfortunately no comment saying what the pin is for, but blame says @philippjfr added it, so he might remember :) Meanwhile, I think the spacing around package and version is (unfortunately) significant - could you match it to the other examples? (It's used not only by setuptools.)

phantomjs is no longer on pyPI and didn't seem to be necessary for generating the docs

I can no longer remember the current situation, but nbsite (used for panel's docs) lists phantomjs as an optional dependency required for building the gallery. Philipp will say definitively, but I'm going to guess that it is still required.

At this point, you can probably stop reading. However, there are a number of annoying asides about the above I'd like to note:

  • Panel should probably just list nbsite[refman, gallery] (or whatever options it needs) as a dependency rather than listing nbsite and also optional dependencies of nbsite. Note: I don't think we should do that in this PR; it should be done when upgrading to pyctdev 0.7 in future.
  • I'm not sure phantomjs has ever been installable with pip. Presumably this dependency was listed for tools that can install it (e.g. conda); I guess we didn't ever expect anyone to try to install the doc building requirements using pip :) We should fix that, but it's a bit complicated. For example, selenium is also listed next to phantomjs - and although pip install selenium runs without error, it won't get you a working selenium install (as far as I know), as it's just the python bindings on pypi. This kind of thing is a constant problem with language vs "system"/"ecosystem" packagers. We should make sure we don't block pip (i.e. not do as we have done here, including dependencies for pip that pip can't install), but again I don't think we should do that yet (it would be part of upgrading to pyctdev 0.7).

@philippjfr
Copy link
Member

There's unfortunately no comment saying what the pin is for, but blame says @philippjfr added it, so he might remember :)

Last I remember we needed a very specific build of VTK compiled against libmesa, which made it possible to run the example without a windowing system on CI.

@a-recknagel
Copy link
Contributor Author

Happy to help, although it looks like I might have caused you more work with this PR than I saved.

build backend

I don't have experience with pyproject.toml and setuptools, I only use it in conjunction with poetry. But I'm fairly sure that it should never be listed in MANIFEST.in, something about cleanly separating packaging metadata and source which was one of the more important reasons for PEP 518 in the first place. In case of sdist distributions the build backend is in charge of providing a build script that applies the packaging data in a setup.py fashion, and pyproject.toml is still not supposed to be part of the tarball.

I only added setuptools.build_meta because my setuptools is recent enough to pick up pyproject.tomls and was complaining about it being unspecified and defaulting to setuptools - I can research more on the matter in order to be certain what it exactly does and if it can cause breaks with older versions.

vtk

It would be nice if it could be asserted if vtk8.1.1 is really necessary, in particular since python3.7 is listed as supported. Another alternative would be to bug the vtk people to release 8.1.1 for python3.7 to pyPI. And I rolled the additional space back, learned something scary/new right there.

the system requirements situtation

Doing a pip install will always have the issue of missing non-python packages, no way around that without conda (this issue as a whole would be part of #706 and adding manual install instructions to a COLLABORATING.md, so it's fine if it isn't handled in this part of the setup). I should have listed my system dependencies in the "environment" section though, which I added right now (but I might have forgotten something, those installs are weeks ago already and my memory goes barely a handful of days back). But if a dev-pip-install is supposed to work, the lists behind the *_requires may only refer to pip-installable packages, anything else breaks it. I can try to run a test with conda tomorrow if it still works despite removing phantomjs.

I'm not sure phantomjs has ever been installable with pip.

It was, back in 2012. There is phantomjs-binary, but that's probably not an option, and as you said also a different issue alltogether.


All in all I think v0.7.0rc3 is good, these changes are not needed since they only deal with the dev setup. The vtk situation should be resolved somehow, but doesn't feel like something blocking to me.

@ceball
Copy link
Member

ceball commented Nov 18, 2019

But I'm fairly sure that it should never be listed in MANIFEST.in, something about cleanly separating packaging metadata and source which was one of the more important reasons for PEP 518 in the first place.

Not saying it makes sense :) Just that things like pypa/setuptools#1650 (from
January) make me worry about things going wrong if someone doesn't have a recent setuptools. If you could find out somehow, that would be great. (E.g. do we need a higher min setuptools in there?)

For now, I think (but am not the authority here!) that we should restore phantomjs and the existing vtk pin in this PR unless you can demonstrate the website build working on CI (i.e. running without error, producing correct site) without them. You might need help to make the necessary builds happen to do that, I'm not sure. And meanwhile we'll accept that nobody can pip install the doc optional extras for now. However, that's an important issue, so I think we should move discussion over to #706 as you say. We do care about #706; sorry nobody's responded there yet.

Does that seem reasonable?

@philippjfr
Copy link
Member

I can no longer remember the current situation, but nbsite (used for panel's docs) lists phantomjs as an optional dependency required for building the gallery.

This is correct, it is still required for thumbnailing bokeh plots. Most thumbnails are downloaded at this point but there's at least a couple that are auto-generated and I would expect to fail if phantomjs isn't available.

@a-recknagel
Copy link
Contributor Author

a-recknagel commented Nov 19, 2019

Well, I checked again and it seems I was plain wrong and the pyproject.toml needs to be part of an sdist after all. In that case it should be included in MANIFEST.in, but given that all of this is a bit uncertain right now and all other changes need to be rolled back, this PR should just be closed I think. I can look more into this, but then in the context of #706. No worries about slow responses there, I am patient =)

@ceball
Copy link
Member

ceball commented Nov 19, 2019

Ok, I've closed this PR for now. But...

In theory, https://github.com/pyviz-dev/pyctdev/issues is where we track packaging issues across all our projects. Although there's code in that repo, you don't have to pay attention to it :) That issue tracker is also probably where we should discuss #706 since it also affects all our projects, but it's apparently not possible to transfer issues between orgs so I can't move it.

@ceball ceball closed this Nov 19, 2019
@philippjfr philippjfr deleted the ar/py3.7_build_amendmends branch September 20, 2021 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants