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

Lazy reader imports #4337

Merged
merged 6 commits into from
Apr 27, 2023
Merged

Lazy reader imports #4337

merged 6 commits into from
Apr 27, 2023

Conversation

akaszynski
Copy link
Member

@akaszynski akaszynski commented Apr 25, 2023

We did an OSS scan within pyansys/pyfluent and discovered a few vulnerabilities. One of those was a TIFF reader from VTK, and it's entirely possible to simply remove the offending library from vtk.

The only issue with this is that we import the library on import, which inspired me to consider if we can just lazy import most, if not all, of our libraries since we now require VTK v9 or greater.

For now, this PR just lazy imports vtkmodules.vtkIOImage.vtkTIFFReader, but is there any reason why we can't just do it for all the modules? This has several advantages:

  • We only import what we use. If you are only post-procesing and don't actually plot, you shouldn't have to import libGLU. This was brought up in Be able to import only VTK classes that do not need OpenGL #3283 and this has even led to people not using PyVista because we import the kitchen sink
  • Improve import speed.
  • Reduced risk to potential upstream vulnerabilities.

As such, I'd suggest that we rethink our imports and actually get away from using _vtk and simply import directly from vtkmodules. This was only really implemented to support backwards compatibility between vtk v8 and v9 and we can now get rid of this technical debt.

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.

I'm +1 on overall intent and am fine for this limited implementation. Possibly future looking comment: if we are going forward with lazy importing everything, or importing at point of use, then we should think about helper functions that avoid having this boilerplate for each reader.

pyvista/utilities/reader.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 25, 2023

Codecov Report

Merging #4337 (2a3bed3) into main (b9ee9b9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #4337   +/-   ##
=======================================
  Coverage   95.78%   95.78%           
=======================================
  Files          97       97           
  Lines       20773    20800   +27     
=======================================
+ Hits        19897    19924   +27     
  Misses        876      876           

@akaszynski akaszynski changed the title Lazy reader imports - TIFF Lazy reader imports Apr 25, 2023
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.

One small comment, otherwise LGTM

pyvista/utilities/reader.py Show resolved Hide resolved
@akaszynski akaszynski merged commit 4cf2f30 into main Apr 27, 2023
24 checks passed
@akaszynski akaszynski deleted the feat/lazy-import-reader branch April 27, 2023 14:38
@akaszynski akaszynski mentioned this pull request Apr 30, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Changes that enhance the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants