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

Improve PyVista Dependency: Use vtk-base instead of full VTK #76

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

basnijholt
Copy link

@basnijholt basnijholt commented Apr 18, 2023

Due to patenting concerns related to FFmpeg, it is not feasible for me or others in a similar situation to install the full VTK package for work-related purposes. It is likely that many others face this issue, though they may not be aware of it.

To address this problem, we have separated VTK into two packages: vtk-base and vtk-io-ffmpeg. This allows users to install only the components they require without potential legal complications.

In light of this, I propose that pyvista should depend on vtk-base instead of the full VTK package. Users who require FFmpeg functionality can simply install vtk-io-ffmpeg separately. This change would make pyvista more accessible to those facing similar patent constraints.

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@banesullivan
Copy link
Contributor

I want to let this sit for a while before we merge. During this time, I'd like to reflect on:

  1. What exactly are the issues with the ffmpeg patent? I haven't seen this before, so I want to understand the problem in detail -- this is on me.
  2. How will this affect the majority of PyVista users? Will this introduce significant confusion to PyVista's installation for a majority of users while only benefiting a small minority limited by the legal complications?
  3. Are there better ways to specify optional dependencies in conda such that the default would still use vtk and only if requested could users depend on vtk-base.
  4. Will PyVista gracefully handle the missing ffmpeg module?

@banesullivan
Copy link
Contributor

I simply want to give this some time for the dust to settle -- otherwise, I'm +1 for this

@banesullivan
Copy link
Contributor

Will PyVista gracefully handle the missing ffmpeg module?

It should since the PyPI wheel has FFMPEG disabled

@basnijholt
Copy link
Author

basnijholt commented Apr 18, 2023

What exactly are the issues with the ffmpeg patent? I haven't seen this before, so I want to understand the problem in detail -- this is on me.

I can only speak for my situation. I work at Microsoft and our systems are automatically flagging anything that depends on VTK (so PyVista too) because FFMPEG contains the AV1 and VP9 codecs which we cannot install.

How will this affect the majority of PyVista users? Will this introduce significant confusion to PyVista's installation for a majority of users while only benefiting a small minority limited by the legal complications?

Are there better ways to specify optional dependencies in conda such that the default would still use vtk and only if requested could users depend on vtk-base.

There are no ways to ignore hard dependencies.

I now realize that because of imageio, ffmpeg is always added as a dependency.

There are two options:

  • adding a build matrix, one without ffmpeg dependencies and one with
  • splitting up the pyvista package into pyvista-base (with bare dependencies), pyvista-imageio, and pyvista which depends on both.

I think the latter implementation is preferred and I can do the work. This should not affect the users in any way, except they can decide to install pyvista-base or pyvista.

@akaszynski
Copy link
Contributor

@basnijholt, thanks for clarifying that.

I now realize that because of imageio, ffmpeg is always added as a dependency.

If it's just imageio that's the issue, I think we can find a workaround for this. In fact, imageio is used only in a handful of locations in pyvista and it's all imported at runtime.

If this is a barrier to adoption, I'm all for making imageio optional.

Opened pyvista/pyvista#4302 to consider this.

@banesullivan
Copy link
Contributor

I agree; we should be able to make imageio a soft dependency in PyVista directly and not need to split this feedstock at all

@FirefoxMetzger
Copy link

I now realize that because of imageio, ffmpeg is always added as a dependency.

Actually, this statement is not correct. ImageIO has two base dependencies: pillow and numpy. Everything else is optional and shouldn't be installed unless you explicitly request it.

@basnijholt Could you share how you determined the dependencies? Maybe there is a bug in ImageIO, in which case I would like to fix it sooner rather than later :)

On the note of FFMPEG, ImageIO has two plugins that use it as a backend: (1) pyav and (2) imageio-ffmpeg. PyAV is a set of Cython bindings into FFMPEGs C-API and imageio-ffmpeg uses a dedicated binary which is called via subprocesses. Both have a hard dependency on FFMPEG to function correctly, but I know that "how the binary is accessed" is sometimes important in these discussions so highlighting this difference may be useful here. At the same time, both are optional dependencies of ImageIO itself, so unless pyvista has a hard requirement to process video (which I don't think it does) it should be possible to avoid FFMPEG while installing ImageIO as usual.

Also, fun fact, the only hard dependency of ImageIO is numpy. ImageIO will be happy as long as there is numpy and at least one backend available. Unfortunately, pip (and I think conda, too) doesn't allow a "default optional dependency", i.e., we can't depend on "at least one dependency from the following list of optional dependencies with some default should a user select none". If we could, we would drop the hard dependency on pillow and make it semi-optional. Since we can't we keep it around to have some means of reading images after installation. To work around this, I have seen people install numpy, perform a bare installation of ImageIO, and then just install the backends they want/need (although this is very rare).


To qualify the above statements: I am the primary maintainer of ImageIO :)

@basnijholt
Copy link
Author

@FirefoxMetzger, after looking at this again, I must have misread this in the recipe/meta.yaml, where I didn't realize that this imageio-ffmpeg only appears in the test dependencies.

My apologies for spreading misinformation 😅

@basnijholt basnijholt closed this Apr 26, 2023
@basnijholt basnijholt reopened this Apr 26, 2023
@basnijholt
Copy link
Author

@conda-forge-admin, please rerender

@github-actions
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/pyvista-feedstock/actions/runs/4805286039.

Copy link
Contributor

@banesullivan banesullivan left a comment

Choose a reason for hiding this comment

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

+1 for this! I haven't tested the vtk-base package myself, so trusting @basnijholt that this is fully compatible

@basnijholt
Copy link
Author

Awesome! So can we merge then? :-)

@banesullivan
Copy link
Contributor

banesullivan commented May 2, 2023

I'll merge today! Wanted to give folks time to chime in and I want to triple check the vtk-base package

Copy link
Contributor

@banesullivan banesullivan left a comment

Choose a reason for hiding this comment

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

I've verified vtk-base generally works with PyVista (didn't run test suite though due to pyvista/pyvista#3474)

However, I have given this more thought, and I am now concerned that merging this will effectively limit VTK to >=9.2.6, which may be too inflexible for most users and will create conflicts if other packages also depend on vtk or pin vtk. This will likely introduce major problems for users with non-trivial environments, and conda will not be able to resolve the dependencies.

With this in mind, I'd like to make sure recipes that use PyVista have a chance to chime in here. Those that come to mind are:

  • @conda-forge/mne, @conda-forge/mne-connectivity, etc (cc @larsoner)
  • @conda-forge/pyvistaqt
  • @conda-forge/pyaedt
  • @conda-forge/gempy
  • @conda-forge/gemgis
  • @conda-forge/puma (this will majorly conflict since they also have vtk in their recipe)
  • @conda-forge/ansys-mapdl-reader, etc (cc @akaszynski)
  • @conda-forge/geovista (pins vtk)
  • ... the list goes on

This makes me think that vtk-base needs to be "opt-in" rather than "opt-out" by default. Or perhaps this justifies a separate pyvista-base recipe indeed

@larsoner
Copy link
Contributor

larsoner commented May 3, 2023

which may be too inflexible for most users and will create conflicts if other packages also depend on vtk or pin vtk.

Yes, in our MNE installers we currently have to pin to 9.2.5 for some unknown reason on macOS (I still need to investigate why). So pinning to 9.2.6 for vtk would mean that we'd also pin PyVista on that installer. But we use pretty basic features so it's probably okay.

Or perhaps this justifies a separate pyvista-base recipe indeed

From what (admittedly little) I've seen and would expect, typically having a whatever-base package implies that whatever's requirements are a superset of those of whatever-base. So in this standard (and IMO reasonable/correct) scheme, pyvista would require pyvista-base. To me it's weird if pyvista-base had more restrictive requirements than pyvista.

@hoechenberger
Copy link
Member

To me it's weird if pyvista-base had more restrictive requirements than pyvista.

I'd like to second this.

@bjlittle
Copy link

bjlittle commented May 3, 2023

@banesullivan Thanks for the heads up.

I'll check whether the issues we were experiencing that cause us to pin to vtk=9.2.2 are still live 👍

@whophil
Copy link
Contributor

whophil commented May 5, 2023

Hi @basnijholt , in light of the issues with requiringvtk-base in PyVista, I wonder if there is another way to do this with modifications to the vtk feedstock instead.

The VTK feedstock currently produces the following packages:

  • vtk (a metapackage requiring)
    • vtk-base
    • vtk-io-ffmpeg

Right now the build string is used to choose between qt, osmesa, egl variants.

Suppose the build string was actually produced as the product of (ffmpeg, noffmpeg) and (qt, osmesa, egl) and . Then the vtk metapackage would have variants with build strings like:

  • ffmpeg_qt
  • noffmpeg_qt
  • ffmpeg_osmesa
  • noffmpeg_osmesa

The recipe itself would just modify the vtk metapackage, e.g.:

  - name: vtk
    build:
      string: {{ build_string }}
      run_exports:
        - {{ pin_subpackage('vtk', max_pin='x.x.x') }}
    requirements:
      build: []
      run:
        - {{ pin_subpackage("vtk-base", exact=True) }}
        - {{ pin_subpackage("vtk-io-ffmpeg", exact=True) }}  # [not win]  <-- skip this line if ffmpeg option is False

Then pyvista could continue to just depend on vtk without any changes, and the user could opt for an ffmpeg_* variant or noffmpeg_* variant in their own environment using the build string.

e.g.,

conda install -c conda-forge pyvista 'vtk=*=*noffmpeg*'

@basnijholt
Copy link
Author

@whophil, that is actually precisely what I had done originally in conda-forge/vtk-feedstock#282, however, @hmaarrfk convinced me that this wasn't a good solution in conda-forge/vtk-feedstock#282 (comment).

@whophil
Copy link
Contributor

whophil commented May 5, 2023

Thanks for the background @basnijholt

It seems that updates to build strings could be combined with the split package approach that has now been merged.

This would circumvent the "doubling the number of builds" penalty that @hmaarrfk mentioned in that thread, as the build strings would only create more variants of the vtk metapackage but not actually do any more compiling.

The concerns about adding too many globs to the build strings are certainly valid, but using the standard prefix for FFMPEG and noffmpeg_* prefix for no-FFMPEG seems sensible.

@hmaarrfk
Copy link

hmaarrfk commented May 5, 2023

Generally speaking, dependencies are quite aggressive in conda-forge.

If people want the older version of PyVista, i think it is ok for them to get an older version of vtk.

If you want to retroactively build specific versions of VTK backporting might be ok.

But for now, i feel like forcing this update for new installations is ok.

I typically think that failing up is ok, rather than building too many unmaintainable hacks.

Is there a specific older version of VTK that you are interested in so that we can likely try to backport some patches for it?

This kind of package transition is somewhat tricky. I opened an issue conda-forge/conda-forge.github.io#1894
And I'm taking people general silence as indication that the answer isn't easy.

An idea if you absolutely need older version of VTK to be compatible, you can have one build starting at build number 200, that depends on vtk-base, the other build, starting at 0, that depends on vtk.

I think that build strings are REALLY overloaded and the requirements on generating them correctly are very loose, and hard to find (in my mind).

I'm mostly trying to propose other alternatives that i believe are more maintainable.

@bjlittle
Copy link

@banesullivan Apologies for the delay in getting back.

Good news! Basically I'm unpinning geovista from vtk=9.2.2 👍

I tested with the latest pyvista 0.39 and vtk, vtk-base, vtk-io-ffmpeg 9.2.6 and the segmentation fault we were experiencing is no longer an issue... which is a major relief.

HTH 😄

@whophil
Copy link
Contributor

whophil commented May 14, 2023

Maybe GitHub is reporting the diff wrong, but on my side this looks like it still requires vtk in addition to vtk-base ?

@basnijholt
Copy link
Author

You are right, thanks for paying attention. I think I incorrectly solved the merge conflict. I'll fix it 👌

@basnijholt
Copy link
Author

Is there any thing I can do to get this moving? 😄

@banesullivan
Copy link
Contributor

I'm feeling a bit lost in the sauce on this one, but I have concerns about it constraining which version of VTK PyVista users can use, considering PyVista targets back to VTK 9.0

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.

9 participants