-
Notifications
You must be signed in to change notification settings - Fork 5
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
Conversation
Codecov Report
@@ 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. |
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 I'd like to see if pyvista/pyvista#3944 could be resolved in a better way before merging this. |
I thought about this as well... and indeed I'm not thrilled to be breaking a pretty clear ecosystem convention.
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 |
I've already had some issues regarding the 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. |
@pyvista/developers, any further thoughts? |
Given this:
If we could have a default for So I'm +1 then. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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 👍
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 newvtk_osmesa
wheels for CI and we need to be able to havepytest-pyvista
inrequirements.txt
without a--no-deps
option. Problem is, this pulls/checks pyvista and thus pullsvtk
. This provides an issue if using thevtk_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 needpyvista
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.