chore: Drop napari<0.6 and pydantic<2#1406
Merged
Merged
Conversation
Contributor
Reviewer's GuideDrops support for older napari (<0.6.x-ish) and pydantic 1.x, simplifying napari-version conditionals, updating runtime/test dependencies and CI/tox matrices to target newer napari/Qt/Pydantic stacks, and aligning internal utilities and settings handling with the new APIs. Flow diagram for updated napari plugin settings path resolutionflowchart TD
A["get_settings called"] --> B{"_SETTINGS is None?"}
B -- No --> G["Return existing _SETTINGS"]
B -- Yes --> C["napari_settings = napari_get_settings"]
C --> D{"napari_settings.config_path is not None?"}
D -- Yes --> E["save_path = dirname(napari_settings.config_path)"]
D -- No --> F["save_path = user_config_dir(napari, False)"]
E --> H["_SETTINGS = PartSegNapariSettings(join(save_path, PartSeg_napari_plugins))"]
F --> H
H --> I["_SETTINGS.load"]
I --> G["Return _SETTINGS"]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughRemoves Pydantic-v1 CI variants and napari-version branches, updates dependency pins and CI/tox matrices, migrates model-copy and Pydantic hooks to v2, simplifies napari runtime code (themes, colormaps, active layer/coordinates), and updates tests to match new APIs. ChangesDrop Pydantic v1 variants and older napari version support
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
.github/workflows/test_napari_widgets.ymltheqt_backendmatrix is set to a single string"PyQt5, PySide2"instead of two separate entries (e.g.["PyQt5", "PySide2"]), which will prevent the matrix from expanding as intended. - In
test_napari_image_view.image_view,QtViewer._instances.clear()is now called unconditionally; consider keeping a defensive check (e.g.hasattr(QtViewer, "_instances")) in case this private attribute changes or is removed in future napari versions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `.github/workflows/test_napari_widgets.yml` the `qt_backend` matrix is set to a single string `"PyQt5, PySide2"` instead of two separate entries (e.g. `["PyQt5", "PySide2"]`), which will prevent the matrix from expanding as intended.
- In `test_napari_image_view.image_view`, `QtViewer._instances.clear()` is now called unconditionally; consider keeping a defensive check (e.g. `hasattr(QtViewer, "_instances")`) in case this private attribute changes or is removed in future napari versions.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
napari<6 and pydantic<2napari<6 and pydantic<2
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/base_test_workflow.yml (1)
1-45:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical workflow input contract break.
The
pydanticworkflow input has been removed from this reusable workflow'sinputsdeclaration, but the caller workflow.github/workflows/tests.yml(line 88) still passespydantic: ${{ matrix.pydantic }}when invoking this workflow. This will cause a workflow validation error at runtime.Either:
- Add the
pydanticinput back to this workflow'son.workflow_call.inputssection (even if unused), or- Update all caller workflows to stop passing the
pydanticparameter.Based on context snippet 1 from
.github/workflows/tests.yml:83-90.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/base_test_workflow.yml around lines 1 - 45, The workflow's inputs removed the pydantic parameter but callers still pass pydantic, causing a validation error; restore a pydantic input in the on.workflow_call.inputs block by adding an entry named pydantic (e.g., required: false, type: string, default: ""), or if you prefer boolean use type: boolean and default: false — ensure the input name is exactly "pydantic" so callers (e.g., the invocation in tests.yml using matrix.pydantic) validate without errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test_napari_widgets.yml:
- Line 40: The qt_backend matrix currently lists a single string "PyQt5,
PySide2" instead of two separate entries; update the workflow matrix entry for
qt_backend to be a proper YAML array of two elements (e.g., ["PyQt5",
"PySide2"]) so the job runs once per backend; locate and edit the qt_backend key
in the matrix section to replace the comma-separated single string with separate
quoted strings.
---
Outside diff comments:
In @.github/workflows/base_test_workflow.yml:
- Around line 1-45: The workflow's inputs removed the pydantic parameter but
callers still pass pydantic, causing a validation error; restore a pydantic
input in the on.workflow_call.inputs block by adding an entry named pydantic
(e.g., required: false, type: string, default: ""), or if you prefer boolean use
type: boolean and default: false — ensure the input name is exactly "pydantic"
so callers (e.g., the invocation in tests.yml using matrix.pydantic) validate
without errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 43315f17-e549-4568-8ec8-f8f547de9ad1
📒 Files selected for processing (22)
.github/workflows/base_test_workflow.yml.github/workflows/test_napari_widgets.yml.github/workflows/tests.yml.github/workflows/upgrade-dependencies.ymlpackage/PartSeg/common_backend/load_backup.pypackage/PartSeg/common_gui/error_report.pypackage/PartSeg/common_gui/napari_image_view.pypackage/PartSeg/plugins/napari_io/loader.pypackage/PartSeg/plugins/napari_widgets/_settings.pypackage/PartSeg/plugins/napari_widgets/lables_control.pypackage/PartSegCore/utils.pypackage/tests/test_PartSeg/test_napari_image_view.pypackage/tests/test_PartSeg/test_napari_io.pypackage/tests/test_PartSeg/test_napari_widgets.pypackage/tests/test_PartSeg/test_viewer.pypyproject.tomlrequirements/constraints_py3.10_pydantic_1.txtrequirements/constraints_py3.11_pydantic_1.txtrequirements/constraints_py3.12_pydantic_1.txtrequirements/constraints_py3.13_pydantic_1.txtrequirements/constraints_py3.14_pydantic_1.txttox.ini
💤 Files with no reviewable changes (8)
- requirements/constraints_py3.10_pydantic_1.txt
- package/PartSeg/plugins/napari_widgets/lables_control.py
- requirements/constraints_py3.13_pydantic_1.txt
- requirements/constraints_py3.11_pydantic_1.txt
- package/tests/test_PartSeg/test_napari_io.py
- .github/workflows/upgrade-dependencies.yml
- requirements/constraints_py3.12_pydantic_1.txt
- .github/workflows/tests.yml
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package/tests/test_PartSeg/test_napari_image_view.py (1)
67-67:⚠️ Potential issue | 🟠 MajorFix napari teardown:
QtViewer._instancesmay not exist in napari 0.6.2
In napari 0.6.2,_instancesis not defined onnapari.qt.QtViewer(instance tracking is handled byViewer/_QtMainWindow).QtViewer._instances.clear()can therefore raiseAttributeErrorduring fixture teardown and fail all tests using this fixture.🛡️ Defensive alternative
- QtViewer._instances.clear() + if hasattr(QtViewer, "_instances"): + QtViewer._instances.clear()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package/tests/test_PartSeg/test_napari_image_view.py` at line 67, The teardown calls QtViewer._instances.clear() which raises AttributeError in napari 0.6.2 because QtViewer may not have _instances; update the fixture to defensively clear instances only if the attribute exists—check hasattr(QtViewer, "_instances") or wrap the clear call in a try/except AttributeError around QtViewer._instances.clear() so tests don't fail when napari.qt.QtViewer lacks _instances.
🧹 Nitpick comments (1)
.github/workflows/test_napari_widgets.yml (1)
34-48: Consider adding an explicit permissions block to the workflow.The static analysis tool flagged that this workflow uses default permissions because no
permissions:block is defined at the workflow level. For security hardening, consider adding an explicit permissions block that grants only the minimum required permissions for the workflow's operations.Example permissions block
Add at the workflow level (after the
concurrency:block):permissions: contents: read pull-requests: readAdjust the specific permissions based on what the workflow actually needs to access.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test_napari_widgets.yml around lines 34 - 48, Add an explicit top-level permissions block to the workflow to avoid default broad permissions; in the workflow that defines the "test-qt5" job add a permissions: stanza (placed after the concurrency: block) and grant only the minimal scopes needed (for example contents: read and pull-requests: read) and adjust as required for any additional actions the job uses so the workflow no longer relies on default permissions.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@package/tests/test_PartSeg/test_napari_image_view.py`:
- Line 67: The teardown calls QtViewer._instances.clear() which raises
AttributeError in napari 0.6.2 because QtViewer may not have _instances; update
the fixture to defensively clear instances only if the attribute exists—check
hasattr(QtViewer, "_instances") or wrap the clear call in a try/except
AttributeError around QtViewer._instances.clear() so tests don't fail when
napari.qt.QtViewer lacks _instances.
---
Nitpick comments:
In @.github/workflows/test_napari_widgets.yml:
- Around line 34-48: Add an explicit top-level permissions block to the workflow
to avoid default broad permissions; in the workflow that defines the "test-qt5"
job add a permissions: stanza (placed after the concurrency: block) and grant
only the minimal scopes needed (for example contents: read and pull-requests:
read) and adjust as required for any additional actions the job uses so the
workflow no longer relies on default permissions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e5bdb6b-fe70-4dc2-9d44-193ac5b25946
📒 Files selected for processing (4)
.github/project_dict.pws.github/workflows/test_napari_widgets.yml.github/workflows/tests.ymlpackage/tests/test_PartSeg/test_napari_image_view.py
💤 Files with no reviewable changes (1)
- .github/workflows/tests.yml
✅ Files skipped from review due to trivial changes (1)
- .github/project_dict.pws
napari<6 and pydantic<2napari<0.6 and pydantic<2
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1406 +/- ##
===========================================
- Coverage 92.72% 92.56% -0.16%
===========================================
Files 211 211
Lines 33273 33224 -49
===========================================
- Hits 30851 30754 -97
- Misses 2422 2470 +48 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package/PartSegImage/channel_class.py`:
- Around line 11-13: The JSON schema claims Channel accepts a non-empty string
but core validation allows empty strings; update the Channel validation to
enforce the non-empty string contract by modifying the pydantic core schema
returned from Channel.__get_pydantic_core_schema__ to use
core_schema.str_schema(min_length=1) (and mirror this constraint in check_type
so it raises for ""), and ensure Channel(...) construction still raises a
ValueError for empty strings with a clear message; also keep
__get_pydantic_json_schema__ description consistent with the tightened core
schema.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 00a8ff77-35b0-4931-999a-f107b62bfc2a
📒 Files selected for processing (2)
package/PartSeg/plugins/napari_widgets/_settings.pypackage/PartSegImage/channel_class.py
🚧 Files skipped from review as they are similar to previous changes (1)
- package/PartSeg/plugins/napari_widgets/_settings.py
|
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Raise the supported napari and pydantic baselines, removing compatibility paths for older napari versions and simplifying configuration and tests accordingly.
Enhancements:
Build:
CI:
Tests:
Chores:
Summary by CodeRabbit
Chores
Bug Fixes
Tests