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

Fix builds #3740

Merged
merged 6 commits into from
Dec 23, 2022
Merged

Fix builds #3740

merged 6 commits into from
Dec 23, 2022

Conversation

akaszynski
Copy link
Member

@akaszynski akaszynski commented Dec 22, 2022

Fix our builds with:

  • Downgrade from ubuntu-22.04 to ubuntu-20.04. The slowdown could be for any number of reasons, but I'm guessing it's due to the increased render time by upgrading from Mesa 21.2.6 to Mesa 22.0.5
  • Get rid of a whole mess of test warnings that make debugging pyvista a bit more challenging.
  • Fix a memory leak issue due to extra pointers when converting pyvista_ndarray to vtkPoints with pyvista.vtk_points that only seems to show up on Python 3.11 (LinuxConda build).

More details about the memory leak

When vtk_points is provided with a pyvista_ndarray and deep=False, VTK retails a reference to the pyvista_ndarray within numpy_to_vtk. Since pyvista_ndarray contains a reference to the vtkDataArray, this results in a circular reference since the vtkDataArray is also referenced by the new vtkDataArray contained in the new vtkPoints object.

One way to work around this is to simply set the new vtk.vtkPoints's data to point to the data referred to by the pyvista_ndarray. This way VTK can track references and wif the pyvista_ndarray is deallocated there will still be a reference to the underlying data. However, this doesn't work with strides because (as far as I can tell) VTK doesn't support strides the same way that numpy does. I could WriteVoidPointer, but we would be back at the same place we'd started since void pointers are just that, pointers to a space in memory rather than an object with reference counting.

Voting for get this merged soon since bad builds mean sad developers.

@github-actions github-actions bot added the bug Uh-oh! Something isn't working as expected. label Dec 22, 2022
@codecov
Copy link

codecov bot commented Dec 22, 2022

Codecov Report

Merging #3740 (858f0c8) into main (6ef1a22) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3740      +/-   ##
==========================================
- Coverage   94.02%   93.99%   -0.03%     
==========================================
  Files          82       82              
  Lines       18545    18553       +8     
==========================================
+ Hits        17437    17439       +2     
- Misses       1108     1114       +6     

@akaszynski akaszynski changed the title get rid of test warnings Fix builds Dec 22, 2022
@akaszynski akaszynski added the maintenance Low-impact maintenance activity label Dec 22, 2022
@banesullivan banesullivan merged commit ff6939a into main Dec 23, 2022
@banesullivan banesullivan deleted the fix/eliminate-test-warnings branch December 23, 2022 19:24
@adeak adeak mentioned this pull request Jan 15, 2023
4 tasks
@banesullivan banesullivan mentioned this pull request Feb 1, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Uh-oh! Something isn't working as expected. maintenance Low-impact maintenance activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants