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

HoloViz: the full Holoviews + Panel + Param + Colorcet stack #1063

Closed
wants to merge 1 commit into from

Conversation

leycec
Copy link
Contributor

@leycec leycec commented Mar 11, 2021

This PR finalizes Gentoo support for most of the HoloViz data visualization stack, including:

  • HoloViews, the really succinct visualization API that makes you never want to use Matplotlib ever again.
  • Panel, the pure-Python WebSocket-driven Tornado-backed Bokeh-based framework for stateful reactive data visualization dashboard web apps. Yeah.
  • Param, the declarative decorator API that transforms Panel apps from procedural to object-oriented. It's good.
  • Colorcet, perceptually accurate colormaps leveraged by all of the above.

Thankfully, this overlay already provides the core Bokeh server required by those packages. That makes life easy for everyone.

Tests are omitted. Some of those packages still test with nose, which is dead and soon to be last-rited. More importantly, supporting tests would've been so time-prohibitive I just never would've done it. The tests for HoloViews and Panel are... really involved. If no one objects, I'd like to defer that to later. 🙏

Thanks for a decade of scientific packaging, everyone – especially @AndrewAmmerlaan for his constant dedication to the cause.

Copy link
Member

@thesamesam thesamesam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a great chunk of work, thanks!

I'm not familiar with HoloViz so I'll let someone else approve/merge but I've left some general Python packaging tips. They're mostly nit-picks but I thought I'd say them so you know -- a lot of it was a good excuse to link to helpful docs which we need to better advertise though.

Let me know if you have any questions but otherwise I'll leave it to the experts. Cheers!

EDIT: If you can for any easy ones, please add tests with distutils_enable_tests. If they're not, cool, we'll do it another time (or not bother -- some upstreams don't really intend for downstreams to run them). Sometimes upstreams end up saying pytest but anything will work - no idea if it's the same with nose though.

KEYWORDS="~amd64 ~x86"
IUSE=""

BDEPEND=">=dev-python/setuptools-30.3.0[${PYTHON_USEDEP}]"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can avoid this, it's set by default

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. I thought that might be the case, but wasn't quite sure.

IUSE=""

BDEPEND=">=dev-python/setuptools-30.3.0[${PYTHON_USEDEP}]"
DEPEND="
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these needed at build time? (Sometimes the answer is yes, but it's often no if it's pure Python)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly, yes. setup.py for HoloViz packages is more convoluted than expected (or needed, frankly). The Colorcet setup.py contains this snippet, for example:

import pyct.build

def get_setup_version(reponame):
    ...
    try:
        from param import version
    except:
        version = None
    if version is not None:
        return version.Version.setup_version(basepath, reponame, archive_commit="$Format:%h$")
    else:
        print("WARNING: param>=1.6.0 unavailable. If you are installing a package, this warning can safely be ignored. If you are creating a package or otherwise operating in a git repository, you should install param>=1.6.0.")

We could probably patch out the get_setup_version() call and thus the install-time param import, but... the pyct.build system might dynamically hook into that import somehow.

Their build workflow looks suspiciously fragile to me, so it might be better to just leave this as is.

LICENSE="BSD"
SLOT="0"
KEYWORDS="~amd64 ~x86"
IUSE=""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can drop empty IUSE (this is a nit, I just like giving people the link to PMS so they're aware of it - it's ok to keep it as-is).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Nice nitpick, too. I'm all for dropping extraneous lines.

<email>sci@gentoo.org</email>
<name>Gentoo Science Project</name>
</maintainer>
</pkgmetadata>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add <stabilize-allarches/> if it's pure Python (no compilation and so on)?

(Applies to all packages where appropriate, as with most of these comments - I'l try make clear if it's just one.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I'll do so. Shamefully, this is the first I've heard of <stabilize-allarches/>. I will now use that everywhere.

There's an interesting parallel here with conda-forge, the automated packaging system for third-party Anaconda packages. conda-forge recipes support a similar noarch: python directive:

Noarch packages are packages that are not architecture specific and therefore only have to be built once. Declaring these packages as noarch in the build section of the meta.yaml reduces shared CI resources. Therefore all packages that qualify to be noarch packages, should be declared as such.

KEYWORDS="~amd64 ~x86"
IUSE=""

BDEPEND=">=dev-python/setuptools-30.3.0[${PYTHON_USEDEP}]"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can drop this and IUSE as DISTUTILS_USE_SETUPTOOLS=rdepend includes BDEPEND.

(This isn't true for the RDEPEND variable in general, it's just what the eclass does.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You got it. 👍

KEYWORDS="~amd64 ~x86"
IUSE=""

DEPEND=">=dev-python/param-1.7.0[${PYTHON_USEDEP}]"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely needed at build time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely. I solemnly swear it on my fat Maine Coon cat.

LICENSE="BSD"
SLOT="0"
KEYWORDS="~amd64 ~x86"
IUSE=""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So long, IUSE. We'll miss you.

<email>sci@gentoo.org</email>
<name>Gentoo Science Project</name>
</maintainer>
</pkgmetadata>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ALLARCHES if applicable, remote-ids, maintain yourself if interested?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, yes, I'd rather stab myself with a plastic spork.

LICENSE="BSD"
SLOT="0"
KEYWORDS="~amd64 ~x86"
IUSE=""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drop

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're dead meat, IUSE!

<email>sci@gentoo.org</email>
<name>Gentoo Science Project</name>
</maintainer>
</pkgmetadata>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

allarches if appropriate, remote-ids, maintain yourself if interested?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, yes, spork-stabbing.

@leycec
Copy link
Contributor Author

leycec commented Mar 11, 2021

Thanks for the incisive nitpicks, @thesamesam. I have now levelled up my Python packaging and am grateful for it.

EDIT: If you can for any easy ones, please add tests with distutils_enable_tests.

...you're right. I should do that. Parsing the 100-line [tests] extra in setup.py for Panel and HoloViews made my eyes bleed. I gave up shortly thereafter. But something is better than nothing; as you say, enabling tests for all of the other packages should be trivial.

Let's give that a go. 👍

@Nowa-Ammerlaan
Copy link
Member

This looks good @leycec 👍

While you're changing things anyway, could you please add a Signed-off-by: line to the commit message, otherwise the PR-merging tool will complain: https://www.gentoo.org/glep/glep-0076.html#certificate-of-origin (Using repoman ci instead of git commit will append this line automatically if SIGNED_OFF_BY="" is set in make.conf, another way to do it automatically is to use git commit with the --signoff or -s argument)

@Nowa-Ammerlaan
Copy link
Member

Repoman is telling me that:

RESTRICT.invalid              1
   dev-python/panel/panel-0.11.0.ebuild: network-sandbox

Are you sure this works as intended, because the ebuild manual does not list 'network-sandbox' as a valid entry for the RESTRICT variable? Usually if the tests require network access, RESTRICT="test" is added to just disable the tests.

  dependency.badindev           1
   dev-python/panel/panel-0.11.0.ebuild: BDEPEND: ~amd64(default/linux/amd64/17.0/x32)
['>=net-libs/nodejs-15.11.0']

We should mask dev-python/panel on this profile, since the nodejs dependency is unavailable. The GURU overlay contains a working example on how to mask packages on a specific profile in an overlay, it is a bit of a strange and complex procedure so let me know if you need any help with it: https://github.com/gentoo/guru/tree/master/profiles

add dev-python/{colorcet,holoviews,panel,param,pyct,pyviz_comms}

Signed-off-by: Cecil Curry <leycec@gmail.com>
@leycec
Copy link
Contributor Author

leycec commented Mar 13, 2021

Done and done. Commit 5e52673 resolves all the bad things, including:

  • Added Signed-off-by: line to commit message.
  • Removed IUSE="" everywhere.
  • Removed >=dev-python/setuptools-30.3.0[${PYTHON_USEDEP}] everywhere.
  • Replaced extraneous ${HOMEPAGE} URLs with <remote-id...> tags in all metadata.xml.
  • Added <stabilize-allarches/> to all metadata.xml.
  • Added distutils_enable_tests pytest to dev-python/{colorcet,pyct}. For the other packages:
    • dev-python/param still expects nose. It is shameful.
    • dev-python/pyviz_comms has no tests. It is more shameful.
    • dev-python/{holoviews,panel} have eye-bleeding tests.
  • Masked dev-python/panel on the default/linux/amd64/17.0/x32 profile.
  • Replaced RESTRICT=network-sandbox in dev-python/panel with:
    • In src_prepare(), a sed patch preventing setup.py from invoking npm at installation time.
    • In pkg_postinst(), one elog message for each enabled Python implementation detailing how to manually rebuild Node.js packages after installation.

That should make all the bad things go away. Thanks for the prompt, detailed, and overly generous review, @thesamesam and @AndrewAmmerlaan. I appreciate it. Really.

Also, GURU looks hot – filling a void left by the long-dead Sunrise overlay. Thanks for maintaining that on behalf of the community, @AndrewAmmerlaan. ❤️

The Horror of "RESTRICT=network-sandbox"

For those curious about RESTRICT=network-sandbox, here's a writeup that doesn't matter because we've solved everything but maybe somebody cares.

RESTRICT=network-sandbox is technically a valid RESTRICT feature (as described by man 5 ebuild). I say technically, because that feature is rightly prohibited in the Portage tree. That's probably what repoman was complaining about, except RESTRICT=network-sandbox isn't invalid; it's just bad.

Here's why we needed that before but don't now. For future posterity (and my future self when I bump into this again), here's what Panel is doing:

  • Panel bundles Node.js packages in its tarball. This is good.
  • Panel's unpatched setup.py:
    1. Thinks these bundled Node.js packages are desynchronized in some way with the currently installed version of Bokeh. This is bad.
    2. Invokes npm, the Node.js package manager. This is bad.
    3. npm tries to redownload all of these packages. This is bad.
  • src_prepare() for Panel now elides this harmful behaviour by patching setup.py to avoid performing that logic. This is good.
  • pkg_postinst() for Panel now details how to manually perform that logic with a simple CLI one-liner after installation, just in case we're wrong and somebody actually needs to do this. This is good.

I've verified this works by serving a toy Panel app. It works without having to run that one-liner, which means that Panel is wrong when it thinks its bundled Node.js packages are desynchronized in some way with the currently installed version of Bokeh.

We suspect Panel developers were trying to minimize issue churn on their bug tracker by forcing Node.js rebuilds on each Panel installation, which had the unintended side effect of preventing Panel from being packaged by any system package manager. Woops.

We should file an upstream issue. Sadly, I am lazy.

@leycec
Copy link
Contributor Author

leycec commented Mar 13, 2021

Done and done. Commit 5e52673 resolves all the bad things, including:

  • Added Signed-off-by: line to commit message.
  • Removed IUSE="" everywhere.
  • Removed >=dev-python/setuptools-30.3.0[${PYTHON_USEDEP}] everywhere.
  • Replaced extraneous ${HOMEPAGE} URLs with <remote-id...> tags in all metadata.xml.
  • Added <stabilize-allarches/> to all metadata.xml.
  • Added distutils_enable_tests pytest to dev-python/{colorcet,pyct}. For the other packages:
    • dev-python/param still expects nose. It is shameful.
    • dev-python/pyviz_comms has no tests. It is more shameful.
    • dev-python/{holoviews,panel} have eye-bleeding tests.
  • Masked dev-python/panel on the default/linux/amd64/17.0/x32 profile.
  • Replaced RESTRICT=network-sandbox in dev-python/panel with:
    • In src_prepare(), a sed patch preventing setup.py from invoking npm at installation time.
    • In pkg_postinst(), one elog message for each enabled Python implementation detailing how to manually rebuild Node.js packages after installation.

That should make all the bad things go away. Thanks for the prompt, detailed, and overly generous review, @thesamesam and @AndrewAmmerlaan. I appreciate it. Really.

Also, GURU looks hot – filling a void left by the long-dead Sunrise overlay. Thanks for maintaining that on behalf of the community, @AndrewAmmerlaan. ❤️

The Horror of "RESTRICT=network-sandbox"

For those curious about RESTRICT=network-sandbox, here's a writeup that doesn't matter because we've solved everything but maybe somebody cares.

RESTRICT=network-sandbox is technically a valid RESTRICT feature (as described by man 5 ebuild). I say technically, because that feature is rightly prohibited in the Portage tree. That's what repoman was complaining about, except RESTRICT=network-sandbox isn't invalid; it's just bad.

Here's why we needed that before but don't now. For future posterity (and my future self when I bump into this again), here's what Panel is doing:

  • Panel bundles Node.js packages in its tarball. This is good.
  • Panel's unpatched setup.py:
    1. Thinks these bundled Node.js packages are desynchronized in some way with the currently installed version of Bokeh. This is bad.
    2. Invokes npm, the Node.js package manager. This is bad.
    3. npm tries to redownload all of these packages. This is bad.
  • src_prepare() for Panel now elides this harmful behaviour by patching setup.py to avoid performing that logic. This is good.
  • pkg_postinst() for Panel now details how to manually perform that logic with a simple CLI one-liner after installation, just in case we're wrong and somebody actually needs to do this. This is good.

I've verified this works by serving a toy Panel app. It works without having to run that one-liner, which means that Panel is wrong when it thinks its bundled Node.js packages are desynchronized in some way with the currently installed version of Bokeh.

We suspect Panel developers were trying to minimize issue churn on their bug tracker by forcing Node.js rebuilds on each Panel installation, which had the unintended side effect of preventing Panel from being packaged by any system package manager. Woops.

We should file an upstream issue. Sadly, I am lazy.

@Nowa-Ammerlaan
Copy link
Member

Thanks @leycec

@leycec
Copy link
Contributor Author

leycec commented Mar 13, 2021

Thanks! Just ping me (@leycec) when these need bumping. 👍

@leycec
Copy link
Contributor Author

leycec commented Mar 13, 2021

@philippjfr: Gentoo Linux now quasi-officially packages most of HoloViz. Just thought you'd like to know. 😉

@philippjfr
Copy link

Thanks, really appreciate your efforts here. I'm not following all the details here but let me know if you need me to explain any of the reasoning behind some of the packaging decisions in Panel in particular.

@leycec
Copy link
Contributor Author

leycec commented Mar 13, 2021

Sure thing! I'd intended to open a Panels issue with a few feature requests to simplify packaging by system package managers (e.g., apt under Debian, pacman under Arch). But this is easier, since we're all here and willing. 😄

The big "pain point" for us are setup.py scripts that open network connections, which we prohibit for both security and stability reasons. Panel's setup.py invokes npm, which opens network connections. Cue pain.

Ideally, we'd like Panel's setup.py to avoid downloading Node.js packages. After all, aren't those packages already bundled with Panel? If that's infeasible, we'd like Panel's setup.py to permit callers to conditionally disable that functionality: e.g.,

  • Via an environment variable like NO_NPM=1.
  • Via a command-line option like --no-npm.

Do you think either of those two things might be feasible? If not, no worries. We currently patch Panel's setup.py with a sed one-liner removing all calls to the _build_paneljs() function. That's a bit fragile, so official support at your leisure would be fantastic! 😍

Lastly, it'd also be great if Panel's setup.py allowed callers to specify a system prefix for data_files. Currently, the data_files in your setup.py are relative rather than absolute:

    data_files=[
        # like `jupyter serverextension enable --sys-prefix`
        (
            "etc/jupyter/jupyter_notebook_config.d",

That installs Panel's Jupyter configuration into /usr/etc/jupyter/ rather than /etc/jupyter/. Naturally, Gentoo expects the latter. We'd like Panel's setup.py to permit callers to conditionally specify a system prefix: e.g.,

  • Via an environment variable like SYS_PREFIX="/".
  • Via a command-line option like --sys-prefix="/".

The above data_files declaration would then become:

    data_files=[
        # like `jupyter serverextension enable --sys-prefix`
        (
            f"{sys_prefix}etc/jupyter/jupyter_notebook_config.d",

...or something. We leave this in your capable hands. Thanks for all the data visualization, Phil. HoloViz rocks the science sphere.

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.

4 participants