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

Deprecate and remove PlotterITK #3737

Merged
merged 2 commits into from
Dec 20, 2022
Merged

Deprecate and remove PlotterITK #3737

merged 2 commits into from
Dec 20, 2022

Conversation

banesullivan
Copy link
Member

@banesullivan banesullivan commented Dec 20, 2022

I'm pretty sure no one is actually using this, and it wasn't even registered as a jupyter_backend. Maintaining this is more of a burden than a benefit, and there are much better alternatives to this API... like using itkwidgets directly which supports PyVista data types!

This is in support of #3690

We're going to deprecate and remove this immediately without a transition plan or warning because it is not a core feature and not a proper "jupyter backend"

@banesullivan banesullivan added breaking-change Changes that break backwards compatibility, especially changed default parameters. IPython/Jupyter IPython/Jupyter related topics labels Dec 20, 2022
@banesullivan banesullivan changed the title Deprecate PlotterITK Deprecate and remove PlotterITK Dec 20, 2022
@github-actions github-actions bot added dependencies Pull requests that update a dependency file documentation Anything related to the documentation/website maintenance Low-impact maintenance activity labels Dec 20, 2022
@banesullivan
Copy link
Member Author

banesullivan commented Dec 20, 2022

Further justification: itkwidgets is broken on import and causing CI failures:

==================================== ERRORS ====================================
_____________ ERROR collecting tests/jupyter/test_itk_plotting.py ______________
tests/jupyter/test_itk_plotting.py:11: in <module>
    import itkwidgets
/usr/share/miniconda/envs/pyvista-vtk/lib/python3.8/site-packages/itkwidgets/__init__.py:13: in <module>
    from .widget_viewer import Viewer, view
/usr/share/miniconda/envs/pyvista-vtk/lib/python3.8/site-packages/itkwidgets/widget_viewer.py:14: in <module>
    import itk
/usr/share/miniconda/envs/pyvista-vtk/lib/python3.8/site-packages/itk/__init__.py:28: in <module>
    from itk.support.extras import *
/usr/share/miniconda/envs/pyvista-vtk/lib/python3.8/site-packages/itk/support/extras.py:35: in <module>
    import itk.support.types as itkt
/usr/share/miniconda/envs/pyvista-vtk/lib/python3.8/site-packages/itk/support/types.py:172: in <module>
    ) = itkCType.initialize_c_types_once()
/usr/share/miniconda/envs/pyvista-vtk/lib/python3.8/site-packages/itk/support/types.py:137: in initialize_c_types_once
    _B: "itkCType" = itkCType("bool", "B", np.bool)
/usr/share/miniconda/envs/pyvista-vtk/lib/python3.8/site-packages/numpy/__init__.py:284: in __getattr__
    raise AttributeError("module {!r} has no attribute "
E   AttributeError: module 'numpy' has no attribute 'bool'
=========================== short test summary info ============================
ERROR tests/jupyter/test_itk_plotting.py - AttributeError: module 'numpy' has no attribute 'bool'
!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!

Looks like it is incompatoble with numpy 1.24 which gave ample warning about the built-in type aliases going away

With this in mind, I'm treating this as a HOTFIX and will merge expeditiously

@banesullivan
Copy link
Member Author

CI failure due to needing #3736

@codecov
Copy link

codecov bot commented Dec 20, 2022

Codecov Report

Merging #3737 (39e5cdb) into main (4bc2064) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3737      +/-   ##
==========================================
- Coverage   94.04%   94.02%   -0.02%     
==========================================
  Files          83       82       -1     
  Lines       18643    18545      -98     
==========================================
- Hits        17532    17437      -95     
+ Misses       1111     1108       -3     

@adeak
Copy link
Member

adeak commented Dec 20, 2022

I'm surprised by this breakage since in #3659 (comment) I could get rid of these by upgrading all dependencies...

@adeak
Copy link
Member

adeak commented Dec 20, 2022

I'm also surprised we don't pin the basic dependencies. If numpy were bumped by dependabot we would've seen this coming.

@banesullivan
Copy link
Member Author

I'm also surprised we don't pin the basic dependencies.

Oh huh. We don't pin numpy, do we. At first, I thought this was just that we don't pin the conda environment but yeah we don't pin the core dependencies. We may want to look into that seperately

Copy link
Member

@akaszynski akaszynski left a comment

Choose a reason for hiding this comment

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

Agree with removing this. This has been broken ever since JupyterLab 3.0, and we have way too many juptyer_backends and should start to converge on a consistent backend soon.

Recommending immediate merge to fix the build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Changes that break backwards compatibility, especially changed default parameters. dependencies Pull requests that update a dependency file documentation Anything related to the documentation/website IPython/Jupyter IPython/Jupyter related topics maintenance Low-impact maintenance activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants