search

pyvista / pytest-pyvista

Plugin to test PyVista plot outputs
https://pytest.pyvista.org
MIT License
13 stars 5 forks source link

Remove PyVista from dependencies #41

Closed banesullivan closed 1 year ago

banesullivan commented 1 year ago

Having PyVista as a hard dependency provides a headache for installing targeted versions of both PyVista and VTK.

PyVista has vtk listed as a requirement, and I don't think we want to remove that. We are beginning to use new vtk_osmesa wheels for CI and we need to be able to have pytest-pyvista in requirements.txt without a --no-deps option. Problem is, this pulls/checks pyvista and thus pulls vtk. This provides an issue if using the vtk_osmesa (or other build variant) wheel and will also mess with the pyvista install if the version isn't right.

IMO, if you're using pytest-pyvista, it's obvious you'll need pyvista installed and thus removing it as a dependency here shouldn't be too confusing.

After all, this module is not meant to be directly interfaced with by users but only integrated into existing testing workflows with PyVista.

codecov-commenter commented 1 year ago

Codecov Report

Merging #41 (688bcc0) into main (acbfd2e) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #41   +/-   ##
=======================================
  Coverage   90.21%   90.21%           
=======================================
  Files           2        2           
  Lines          92       92           
=======================================
  Hits           83       83           
  Misses          9        9

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

MatthewFlamm commented 1 year ago

I think there is no real harm in removing this as a dependency directly. As you point out every usage of this tool by definition already requires pyvista to be installed. But looking at other pytest-* packages to gauge user expectations: pytest-mpl includes matplotlib as a dependency. pytest-aiohttp includes aiohttp (by the way this one also has a minimum version). pytest-freezegun requires freezegun (also with minimum version). pytest_httpx requires httpx matching minor version. If we ever need to pin minimum version for pyvista here (it is probably inevitable), it would be better to start from a state of having a dependency with an unspecified version.

I'd like to see if https://github.com/pyvista/pyvista/pull/3944 could be resolved in a better way before merging this.

banesullivan commented 1 year ago

But looking at other pytest-* packages to gauge user expectations:

I thought about this as well... and indeed I'm not thrilled to be breaking a pretty clear ecosystem convention.

I'd like to see if https://github.com/pyvista/pyvista/pull/3944 could be resolved in a better way before merging this.

For posterity, the real issue is: https://github.com/pyvista/pyvista/pull/3704#discussion_r1094927082

AFAIK, there really isn't a good way to handle this since we want to be able to use any vtk-* build variant (e.g., vtk-osmesa, vtk-egl, vtk-openvr, etc.) without removing vtk from PyVista's core dependencies... which I think we should avoid.

AlejandroFernandezLuces commented 1 year ago

I've already had some issues regarding the pyvista dependency in this plugin as well. The main issue I see is that previous versions of pyvista won't work with this plugin, so maybe some people that need to use earlier versions of pyvista won't know which version is compatible with the plugin. It might even lead to people thinking they are using the plugin while they are actually using the test utilities that were used previously in pyvista.

Anyway, if we have to make a tradeoff, I personally prefer to comply with the case you mention, so the plugin can be used in most cases.

banesullivan commented 1 year ago

@pyvista/developers, any further thoughts?

MatthewFlamm commented 1 year ago

Given this:

AFAIK, there really isn't a good way to handle this since we want to be able to use any vtk-* build variant (e.g., vtk-osmesa, vtk-egl, vtk-openvr, etc.) without removing vtk from PyVista's core dependencies... which I think we should avoid.

If we could have a default for extras_require that could be turned off, we could put vtk into that and then could be turned off for niche CI usage (and/or the other vtk flavors could be added as non-default options). But, this didn't exist yet, see https://github.com/pypa/setuptools/issues/1139

So I'm +1 then.