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

Remove PyVista from dependencies #41

Merged
merged 1 commit into from
Feb 7, 2023
Merged

Conversation

banesullivan
Copy link
Member

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
Copy link

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
Copy link
Contributor

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 pyvista/pyvista#3944 could be resolved in a better way before merging this.

@banesullivan
Copy link
Member Author

banesullivan commented Feb 2, 2023

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 pyvista/pyvista#3944 could be resolved in a better way before merging this.

For posterity, the real issue is: pyvista/pyvista#3704 (comment)

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
Copy link
Collaborator

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
Copy link
Member Author

@pyvista/developers, any further thoughts?

@MatthewFlamm
Copy link
Contributor

MatthewFlamm commented Feb 6, 2023

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 pypa/setuptools#1139

So I'm +1 then.

Copy link
Contributor

@MatthewFlamm MatthewFlamm left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Even if it's not an ideal solution, it seems like an overall (large) reduction in dependency difficulties, especially given the workaround/way to get previous behavior is just to tack on pyvista to the deps list 👍

@banesullivan banesullivan merged commit 00b8dc8 into main Feb 7, 2023
@banesullivan banesullivan deleted the maint/no-pyvista-dep branch February 7, 2023 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants