chore: Drop python 3.9 support#1404
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Sorry @Czaki, your pull request is larger than the review limit of 150000 diff characters
Contributor
|
Too much diff to scan? Review this PR in Change Stack to start with the highest-impact changes. 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:
📝 WalkthroughWalkthroughBumps CI and packaging to Python >=3.10, replaces Optional/Union with PEP 604 ChangesPython 3.10+ Migration and Typing Modernization
Estimated code review effort Possibly related PRs
Suggested labels Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
package/PartSeg/plugins/napari_io/loader.py (1)
25-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
adjust_colorreturn contract (generator vs tuple + wrongtuple[float])
Inpackage/PartSeg/plugins/napari_io/loader.py, thelist[int]overload promiseslist[float], but the implementation returns a generator expression; additionally the implementation annotationtuple[float]denotes a 1-element tuple, not an RGB 3-float tuple.Proposed fix
`@typing.overload` def adjust_color(color: str) -> str: ... `@typing.overload` -def adjust_color(color: list[int]) -> list[float]: ... +def adjust_color(color: list[int]) -> tuple[float, float, float]: ... -def adjust_color(color: str | list[int]) -> str | tuple[float]: +def adjust_color(color: str | list[int]) -> str | tuple[float, float, float]: @@ - elif isinstance(color, list): - return (color[i] / 255 for i in range(3)) + elif isinstance(color, list): + return tuple(color[i] / 255 for i in range(3))🤖 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/PartSeg/plugins/napari_io/loader.py` around lines 25 - 37, adjust_color currently promises to return tuple[float] but returns a generator for list inputs and a wrongly-typed annotation; update the function signature to return str | tuple[float, float, float], and replace the generator expression in the list branch with an actual 3-tuple of floats (e.g. compute the three color components divided by 255 and return as a tuple) so callers receive a concrete (r,g,b) tuple rather than a generator and the type annotation matches the runtime shape.package/PartSegCore/analysis/measurement_calculation.py (1)
297-299:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the right subtree when deriving right-side component/area metadata.
Line 298 currently evaluates
tree.lefttwice, so the right branch is ignored and combined metadata can be incorrect.Suggested fix
left_par, left_area = self._get_par_component_and_area_type(tree.left) - right_par, right_area = self._get_par_component_and_area_type(tree.left) + right_par, right_area = self._get_par_component_and_area_type(tree.right)🤖 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/PartSegCore/analysis/measurement_calculation.py` around lines 297 - 299, The code calls self._get_par_component_and_area_type(tree.left) for both left and right, ignoring the right subtree; update the second call to use tree.right so right_par and right_area are derived from the right subtree (i.e., call self._get_par_component_and_area_type(tree.right) when setting right_par, right_area) so the subsequent check using PerComponent.Yes on [left_par, right_par] is correct.package/PartSeg/_roi_mask/stack_settings.py (1)
322-335:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign
get_maskreturn type with actualNonereturn path.When
segmentation is Noneandmask is None, this function returnsNone, but the signature declares-> np.ndarray. Please update the annotation to reflect the real contract.Suggested patch
-def get_mask(segmentation: np.ndarray | None, mask: np.ndarray | None, selected: list[int]) -> np.ndarray: +def get_mask(segmentation: np.ndarray | None, mask: np.ndarray | None, selected: list[int]) -> np.ndarray | None:🤖 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/PartSeg/_roi_mask/stack_settings.py` around lines 322 - 335, The function get_mask can return None when segmentation or mask is None, but its return type is declared as np.ndarray; change the signature to return typing.Optional[np.ndarray] (or Optional[np.ndarray]) so the annotation matches actual behavior, update any import if needed, and adjust the docstring return/rtype lines to reflect Optional[np.ndarray]; locate get_mask in stack_settings.py to apply this change.package/PartSeg/common_gui/colormap_creator.py (1)
663-693:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix declared
BytesIOsupport for colormap JSON save/load
save_location/load_locationsare typed to acceptBytesIO, but the implementation usesopen(save_location, ...)andopen(load_locations[0]), which raisesTypeError: expected str, bytes or os.PathLike object, not BytesIOatpackage/PartSeg/common_gui/colormap_creator.pylines 669 and 692. HandleBytesIOexplicitly (write/read from the buffer) instead of callingopen(...)on it.Proposed fix
@@ - def save( + def save( cls, save_location: str | BytesIO | Path, project_info, parameters: dict | None = None, range_changed=None, step_changed=None, ): - with open(save_location, "w") as f: - json.dump(project_info, f, cls=local_migrator.Encoder, indent=4) + if isinstance(save_location, BytesIO): + payload = json.dumps(project_info, cls=local_migrator.Encoder, indent=4).encode("utf-8") + save_location.write(payload) + return + with open(save_location, "w", encoding="utf-8") as f: + json.dump(project_info, f, cls=local_migrator.Encoder, indent=4) @@ - def load( + def load( cls, load_locations: list[str | BytesIO | Path], range_changed: typing.Callable[[int, int], typing.Any] | None = None, step_changed: typing.Callable[[int], typing.Any] | None = None, metadata: dict | None = None, ) -> Colormap: - with open(load_locations[0]) as f: - return json.load(f, object_hook=local_migrator.object_hook) + source = load_locations[0] + if isinstance(source, BytesIO): + source.seek(0) + return json.loads(source.read().decode("utf-8"), object_hook=local_migrator.object_hook) + with open(source, encoding="utf-8") as f: + return json.load(f, object_hook=local_migrator.object_hook)🤖 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/PartSeg/common_gui/colormap_creator.py` around lines 663 - 693, The save implementation that accepts a save_location parameter and the ColormapLoad.load method currently call open(...) directly which fails for BytesIO; update both to handle BytesIO explicitly: in the save routine (the function with parameter save_location) detect if isinstance(save_location, BytesIO) and write the JSON text to it (e.g., json_str = json.dumps(project_info, cls=local_migrator.Encoder, indent=4) then write json_str.encode() or use a TextIOWrapper), otherwise use open(save_location, "w") as before; in ColormapLoad.load detect if isinstance(load_locations[0], BytesIO) and read from the buffer (decode bytes or use TextIOWrapper) then json.loads(..., object_hook=local_migrator.object_hook), otherwise open the path with open(...) as currently implemented.
🧹 Nitpick comments (1)
package/PartSeg/_roi_analysis/advanced_window.py (1)
369-369: ⚡ Quick winFix
chosen_element_areaannotation to match actual runtime type.
chosen_element_areais used as a measurement node object (accessed via.area/.per_componentand passed asNode.left), sotuple[AreaType, float] | Noneis inconsistent and weakens type safety.Suggested patch
- self.chosen_element_area: tuple[AreaType, float] | None = None + self.chosen_element_area: Leaf | Node | None = None🤖 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/PartSeg/_roi_analysis/advanced_window.py` at line 369, chosen_element_area is annotated as tuple[AreaType, float] | None but at runtime it holds a measurement node object (the object you call .area and .per_component on and pass to Node.left); update its annotation to the actual measurement node class (e.g., AreaMeasurementNode or the class name used for area nodes in your codebase) and import that class, then declare chosen_element_area: Optional[ThatMeasurementNode]; if the concrete class is unclear, use typing.Any (Optional[Any]) as a temporary fallback and replace it with the real type.
🤖 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/tests.yml:
- Around line 148-149: The CI matrix entry for the test_minimal job sets
python_version: "3.9" but uses tox_args: "-e py310-PyQt5-minimal", causing a
mismatch; update the python_version to "3.10" (or otherwise match the py310 tox
environment) so the Python version aligns with the py310 tox environment and the
project's Python 3.9 drop goal—change the python_version field in the same
matrix entry that contains tox_args and any job name or identifier referencing
test_minimal/py310-PyQt5-minimal.
In `@package/PartSeg/common_gui/custom_load_dialog.py`:
- Line 131: Update the typing annotations that use the runtime expression
type(LoadBase) to the static form type[LoadBase] so type checkers recognize
"subclass of LoadBase"; specifically change the load_register parameter in
PLoadDialog.__init__ and any occurrences in IORegister (and related annotations
referencing LoadBase) from type(LoadBase) to type[LoadBase], and adjust
union/collection annotations (e.g., dict[str, type[LoadBase]] | type[LoadBase])
accordingly.
In `@package/PartSeg/common_gui/label_create.py`:
- Around line 332-336: The type hint wrongly advertises BytesIO support for the
load_locations parameter even though the implementation calls open(...) on each
entry; remove BytesIO from the annotated union (change list[str | BytesIO |
Path] to list[str | Path]) in the function signature(s) that define
load_locations and the other two signatures mentioned, and update any
docstrings/comments to match the tightened contract so callers aren't misled
into passing BytesIO objects that will cause a runtime error.
In `@package/PartSeg/common_gui/napari_image_view.py`:
- Around line 732-733: The loop generating grid positions uses zip(...,
itertools.product(...), strict=True) which raises ValueError when product yields
more positions than images; replace this by limiting the generated positions to
the exact number of images and remove strict: use
itertools.islice(itertools.product(range(n_row), repeat=2),
len(self.image_info)) and zip(self.image_info.values(), that_islice) so
translate_2d = np.multiply(scene_size[-2:], pos) still applies; update imports
to include itertools.islice and reference the existing symbols self.image_info,
translate_2d, and n_row in the change.
In `@package/PartSeg/plugins/itk_snap_save/__init__.py`:
- Line 26: The SaveITKSnap.save() signature incorrectly includes BytesIO for
save_location but forwards it to SimpleITK.WriteImage(mask, save_location),
which accepts only filesystem paths; update SaveITKSnap.save() to accept only
path types (e.g., change save_location annotation to str | Path) and remove
BytesIO from the type OR implement conversion of a BytesIO by writing its
contents to a temporary file and passing that temp path to SimpleITK.WriteImage;
update any related docstring and callers to match the new API.
In `@package/PartSeg/plugins/modeling_save/save_modeling_data.py`:
- Line 45: The save_location parameter is annotated to allow BytesIO but the
save() implementation only works with filesystem paths; update the type hints to
restrict save_location to path-like types (e.g., str | Path | os.PathLike) and
remove BytesIO from the union in the save() function signature (and the other
related signatures around the same block), ensuring all internal uses expect a
directory path; alternatively, if you want to keep file-like support, implement
explicit handling in save() to detect a BytesIO and raise a clear error or
implement a different code path for writing multiple files into an archive, but
the minimal fix is to change the annotations to path-like only in
save_modeling_data.py for the save() and related functions.
In `@package/PartSeg/plugins/old_partseg/old_partseg.py`:
- Around line 67-70: The load() signature declares load_locations: list[str |
BytesIO | Path] but the control flow only handles str, TarFile and IOBase so
Path objects fall through to ValueError; update the load() implementation to
detect pathlib.Path (or use os.fspath) and convert Path instances to strings (or
file-like objects as appropriate) before the existing branching, and apply the
same fix to the analogous handling in the other block referenced (lines ~73-83).
Ensure you reference the parameter name load_locations and the load() function
(and the local variable that iterates locations, e.g., location) when making the
change.
In `@package/PartSegCore/analysis/calculation_plan.py`:
- Line 238: name_dict is declared as possibly None but parse_map writes into it
(e.g., item assignments in parse_map), causing a NoneType error on first parse;
update the code that defines/uses name_dict (the variable named name_dict and
the function parse_map) to ensure name_dict is initialized to an empty dict
before any writes (for example, set name_dict = {} if it is None at the start of
parse_map or where name_dict is declared) and apply the same initialization
logic for the other parse paths around the parse_map-related code (the code
block covering the parse_map interactions currently around lines that reference
name_dict).
In `@package/PartSegImage/image.py`:
- Around line 417-420: The list comprehension that builds merged channel arrays
is using the right image's channel data twice (y and reordered y) which drops
this instance's channels; in the comprehension inside the merge routine replace
the first operand with the left image channel variable (x) so it concatenates x
with the reordered y when axis is not 'C'. Locate the comprehension that
iterates over self._channel_arrays and image._channel_arrays (the variables x,
y) and change the first concatenation argument from y to x while keeping the
call to self.reorder_axes(y, image.array_axis_order) and the axis=index
unchanged.
---
Outside diff comments:
In `@package/PartSeg/_roi_mask/stack_settings.py`:
- Around line 322-335: The function get_mask can return None when segmentation
or mask is None, but its return type is declared as np.ndarray; change the
signature to return typing.Optional[np.ndarray] (or Optional[np.ndarray]) so the
annotation matches actual behavior, update any import if needed, and adjust the
docstring return/rtype lines to reflect Optional[np.ndarray]; locate get_mask in
stack_settings.py to apply this change.
In `@package/PartSeg/common_gui/colormap_creator.py`:
- Around line 663-693: The save implementation that accepts a save_location
parameter and the ColormapLoad.load method currently call open(...) directly
which fails for BytesIO; update both to handle BytesIO explicitly: in the save
routine (the function with parameter save_location) detect if
isinstance(save_location, BytesIO) and write the JSON text to it (e.g., json_str
= json.dumps(project_info, cls=local_migrator.Encoder, indent=4) then write
json_str.encode() or use a TextIOWrapper), otherwise use open(save_location,
"w") as before; in ColormapLoad.load detect if isinstance(load_locations[0],
BytesIO) and read from the buffer (decode bytes or use TextIOWrapper) then
json.loads(..., object_hook=local_migrator.object_hook), otherwise open the path
with open(...) as currently implemented.
In `@package/PartSeg/plugins/napari_io/loader.py`:
- Around line 25-37: adjust_color currently promises to return tuple[float] but
returns a generator for list inputs and a wrongly-typed annotation; update the
function signature to return str | tuple[float, float, float], and replace the
generator expression in the list branch with an actual 3-tuple of floats (e.g.
compute the three color components divided by 255 and return as a tuple) so
callers receive a concrete (r,g,b) tuple rather than a generator and the type
annotation matches the runtime shape.
In `@package/PartSegCore/analysis/measurement_calculation.py`:
- Around line 297-299: The code calls
self._get_par_component_and_area_type(tree.left) for both left and right,
ignoring the right subtree; update the second call to use tree.right so
right_par and right_area are derived from the right subtree (i.e., call
self._get_par_component_and_area_type(tree.right) when setting right_par,
right_area) so the subsequent check using PerComponent.Yes on [left_par,
right_par] is correct.
---
Nitpick comments:
In `@package/PartSeg/_roi_analysis/advanced_window.py`:
- Line 369: chosen_element_area is annotated as tuple[AreaType, float] | None
but at runtime it holds a measurement node object (the object you call .area and
.per_component on and pass to Node.left); update its annotation to the actual
measurement node class (e.g., AreaMeasurementNode or the class name used for
area nodes in your codebase) and import that class, then declare
chosen_element_area: Optional[ThatMeasurementNode]; if the concrete class is
unclear, use typing.Any (Optional[Any]) as a temporary fallback and replace it
with the real type.
🪄 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: abcbfd90-5b00-4197-84dc-99c7fc1e96b0
📒 Files selected for processing (91)
.github/workflows/test_napari_widgets.yml.github/workflows/tests.ymlpackage/PartSeg/_roi_analysis/advanced_window.pypackage/PartSeg/_roi_analysis/batch_window.pypackage/PartSeg/_roi_analysis/calculation_pipeline_thread.pypackage/PartSeg/_roi_analysis/export_batch.pypackage/PartSeg/_roi_analysis/image_view.pypackage/PartSeg/_roi_analysis/partseg_settings.pypackage/PartSeg/_roi_analysis/prepare_plan_widget.pypackage/PartSeg/_roi_analysis/profile_export.pypackage/PartSeg/_roi_mask/batch_proceed.pypackage/PartSeg/_roi_mask/main_window.pypackage/PartSeg/_roi_mask/segmentation_info_dialog.pypackage/PartSeg/_roi_mask/stack_settings.pypackage/PartSeg/common_backend/base_argparser.pypackage/PartSeg/common_backend/base_settings.pypackage/PartSeg/common_backend/partially_const_dict.pypackage/PartSeg/common_backend/progress_thread.pypackage/PartSeg/common_gui/advanced_tabs.pypackage/PartSeg/common_gui/algorithms_description.pypackage/PartSeg/common_gui/channel_control.pypackage/PartSeg/common_gui/collapse_checkbox.pypackage/PartSeg/common_gui/colormap_creator.pypackage/PartSeg/common_gui/custom_load_dialog.pypackage/PartSeg/common_gui/custom_save_dialog.pypackage/PartSeg/common_gui/error_report.pypackage/PartSeg/common_gui/image_adjustment.pypackage/PartSeg/common_gui/label_create.pypackage/PartSeg/common_gui/main_window.pypackage/PartSeg/common_gui/mask_widget.pypackage/PartSeg/common_gui/napari_image_view.pypackage/PartSeg/common_gui/napari_viewer_wrap.pypackage/PartSeg/common_gui/stack_image_view.pypackage/PartSeg/common_gui/universal_gui_part.pypackage/PartSeg/plugins/itk_snap_save/__init__.pypackage/PartSeg/plugins/modeling_save/save_modeling_data.pypackage/PartSeg/plugins/napari_io/loader.pypackage/PartSeg/plugins/napari_io/save_mask_roi.pypackage/PartSeg/plugins/napari_io/save_tiff_layer.pypackage/PartSeg/plugins/napari_widgets/algorithm_widgets.pypackage/PartSeg/plugins/napari_widgets/colormap_control.pypackage/PartSeg/plugins/napari_widgets/simple_measurement_widget.pypackage/PartSeg/plugins/napari_widgets/utils.pypackage/PartSeg/plugins/old_partseg/old_partseg.pypackage/PartSegCore/_old_json_hooks.pypackage/PartSegCore/algorithm_describe_base.pypackage/PartSegCore/analysis/batch_processing/batch_backend.pypackage/PartSegCore/analysis/batch_processing/parallel_backend.pypackage/PartSegCore/analysis/calculate_pipeline.pypackage/PartSegCore/analysis/calculation_plan.pypackage/PartSegCore/analysis/io_utils.pypackage/PartSegCore/analysis/load_functions.pypackage/PartSegCore/analysis/measurement_base.pypackage/PartSegCore/analysis/measurement_calculation.pypackage/PartSegCore/analysis/save_functions.pypackage/PartSegCore/class_generator.pypackage/PartSegCore/convex_fill.pypackage/PartSegCore/image_operations.pypackage/PartSegCore/image_transforming/combine_channels.pypackage/PartSegCore/image_transforming/image_projection.pypackage/PartSegCore/image_transforming/interpolate_image.pypackage/PartSegCore/image_transforming/swap_time_stack.pypackage/PartSegCore/image_transforming/transform_base.pypackage/PartSegCore/io_utils.pypackage/PartSegCore/mask/io_functions.pypackage/PartSegCore/mask_create.pypackage/PartSegCore/mask_partition_utils.pypackage/PartSegCore/plugins/__init__.pypackage/PartSegCore/project_info.pypackage/PartSegCore/roi_info.pypackage/PartSegCore/segmentation/algorithm_base.pypackage/PartSegCore/segmentation/restartable_segmentation_algorithms.pypackage/PartSegCore/segmentation/segmentation_algorithm.pypackage/PartSegCore/segmentation/threshold.pypackage/PartSegCore/segmentation/watershed.pypackage/PartSegCore/utils.pypackage/PartSegImage/channel_class.pypackage/PartSegImage/image.pypackage/PartSegImage/image_reader.pypackage/PartSegImage/image_writer.pypackage/tests/test_PartSeg/test_colormap_create.pypackage/tests/test_PartSeg/test_common_backend.pypackage/tests/test_PartSeg/test_common_gui.pypackage/tests/test_PartSegCore/test_algorithm_describe_base.pypackage/tests/test_PartSegCore/test_analysis_batch.pypackage/tests/test_PartSegCore/test_class_generator.pypackage/tests/test_PartSegCore/test_segmentation.pypackage/tests/test_PartSegImage/test_image.pypyproject.tomlrequirements/constraints_py3.9.txtrequirements/constraints_py3.9_pydantic_1.txt
💤 Files with no reviewable changes (3)
- requirements/constraints_py3.9.txt
- requirements/constraints_py3.9_pydantic_1.txt
- package/PartSegCore/plugins/init.py
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)
.github/workflows/tests.yml (1)
107-119:⚠️ Potential issue | 🟠 Major | ⚡ Quick winContradictory matrix include/exclude for Python 3.11 + PySide2.
The
includesection addspython_version: "3.11"withqt_backend: "PySide2"onubuntu-22.04(lines 107–109), but theexcludesection immediately removes allpython_version: "3.11"withqt_backend: "PySide2"combinations (lines 118–119). This results in the included entry being excluded, making it unreachable in the CI matrix.Recommended fix
If Python 3.11 + PySide2 testing is intended, remove the conflicting exclude rule:
exclude: - - python_version: "3.11" - qt_backend: "PySide2" - python_version: "3.12" qt_backend: "PySide2"If Python 3.11 + PySide2 testing is not intended, remove the conflicting include entry:
- python_version: "3.12" qt_backend: "PyQt5" os: "ubuntu-22.04" - - python_version: "3.11" - os: "ubuntu-22.04" - qt_backend: "PySide2" - python_version: "3.11" os: "ubuntu-22.04"🤖 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/tests.yml around lines 107 - 119, The matrix currently both includes and excludes the combination python_version: "3.11" with qt_backend: "PySide2" (ubuntu-22.04) via the include and exclude blocks; decide intent and make the matrix consistent by either removing the exclude entry that matches python_version: "3.11" / qt_backend: "PySide2" so the included case runs, or by removing the include entry for python_version: "3.11" / qt_backend: "PySide2" if that combination should not be tested; update only the include/exclude entries (matrix include/exclude) to eliminate the contradictory rule.
♻️ Duplicate comments (1)
package/PartSeg/plugins/modeling_save/save_modeling_data.py (1)
45-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
save_locationtype allowsBytesIO, but implementation requires filesystem paths.The
save_locationparameter annotation includesBytesIO, but lines 51–54 immediately callos.path.exists()andos.path.isdir()on it, which require path-like objects and will fail at runtime if aBytesIOis passed.Recommended fix
- def save( - cls, - save_location: str | BytesIO | Path, + def save( + cls, + save_location: str | Path, project_info: ProjectTuple,🤖 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/PartSeg/plugins/modeling_save/save_modeling_data.py` at line 45, The annotation for save_location currently allows BytesIO but the implementation unconditionally calls os.path.exists() and os.path.isdir() on save_location; either remove BytesIO from the annotation or add branching to handle both cases: in the function that receives save_location (save_modeling_data.py, parameter save_location) check isinstance(save_location, (str, Path, os.PathLike)) before calling os.path.exists()/os.path.isdir() and treat that branch as the filesystem path flow, and add an elif for isinstance(save_location, BytesIO) to skip filesystem checks and write to the buffer using file-like semantics; update the type hint to reflect both supported types if you keep BytesIO (e.g., str | Path | BytesIO) and ensure any file-opening/writing code uses open(...) only in the path branch and buffer.write(...) in the BytesIO branch.
🤖 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 @.github/workflows/tests.yml:
- Around line 107-119: The matrix currently both includes and excludes the
combination python_version: "3.11" with qt_backend: "PySide2" (ubuntu-22.04) via
the include and exclude blocks; decide intent and make the matrix consistent by
either removing the exclude entry that matches python_version: "3.11" /
qt_backend: "PySide2" so the included case runs, or by removing the include
entry for python_version: "3.11" / qt_backend: "PySide2" if that combination
should not be tested; update only the include/exclude entries (matrix
include/exclude) to eliminate the contradictory rule.
---
Duplicate comments:
In `@package/PartSeg/plugins/modeling_save/save_modeling_data.py`:
- Line 45: The annotation for save_location currently allows BytesIO but the
implementation unconditionally calls os.path.exists() and os.path.isdir() on
save_location; either remove BytesIO from the annotation or add branching to
handle both cases: in the function that receives save_location
(save_modeling_data.py, parameter save_location) check isinstance(save_location,
(str, Path, os.PathLike)) before calling os.path.exists()/os.path.isdir() and
treat that branch as the filesystem path flow, and add an elif for
isinstance(save_location, BytesIO) to skip filesystem checks and write to the
buffer using file-like semantics; update the type hint to reflect both supported
types if you keep BytesIO (e.g., str | Path | BytesIO) and ensure any
file-opening/writing code uses open(...) only in the path branch and
buffer.write(...) in the BytesIO branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9165a7b2-f497-4e19-9b0b-396778b89464
📒 Files selected for processing (19)
.github/workflows/tests.ymlpackage/PartSeg/_roi_analysis/advanced_window.pypackage/PartSeg/_roi_analysis/measurement_widget.pypackage/PartSeg/_roi_mask/stack_settings.pypackage/PartSeg/common_gui/algorithms_description.pypackage/PartSeg/common_gui/custom_load_dialog.pypackage/PartSeg/plugins/modeling_save/save_modeling_data.pypackage/PartSeg/plugins/napari_widgets/mask_create_widget.pypackage/PartSeg/plugins/napari_widgets/measurement_widget.pypackage/PartSeg/plugins/napari_widgets/simple_measurement_widget.pypackage/PartSeg/plugins/napari_widgets/utils.pypackage/PartSegCore/algorithm_describe_base.pypackage/PartSegCore/analysis/measurement_base.pypackage/PartSegCore/analysis/measurement_calculation.pypackage/PartSegCore/color_image/base_colors.pypackage/PartSegImage/image.pypyproject.tomltox.initutorials/tutorial_neuron_types/Neuron_types_example.ipynb
🚧 Files skipped from review as they are similar to previous changes (6)
- package/PartSeg/_roi_mask/stack_settings.py
- package/PartSeg/plugins/napari_widgets/simple_measurement_widget.py
- package/PartSeg/common_gui/custom_load_dialog.py
- package/PartSeg/common_gui/algorithms_description.py
- package/PartSegImage/image.py
- package/PartSegCore/algorithm_describe_base.py
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #1404 +/- ##
===========================================
- Coverage 93.12% 92.72% -0.40%
===========================================
Files 211 211
Lines 33285 33273 -12
===========================================
- Hits 30995 30851 -144
- Misses 2290 2422 +132 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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 (2)
tox.ini (2)
12-13:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the obsolete Python 3.9 mapping.
The
gh-actionssection still contains3.9: py39, which is inconsistent with the PR objective of dropping Python 3.9 support. This mapping should be removed.🧹 Proposed fix
[gh-actions] python = - 3.9: py39 3.10: py310 3.11: py311🤖 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 `@tox.ini` around lines 12 - 13, Remove the obsolete Python 3.9 mapping from the gh-actions table in tox.ini by deleting the line "3.9: py39" so the gh-actions mapping matches the PR's intent to drop Python 3.9 support; ensure only the remaining mappings (e.g., "3.10: py310") remain and that the gh-actions section syntax remains valid after removal.
79-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate the testenv header to include napari_70.
Line 84 adds support for
napari_70: napari==0.7.0, but the testenv header on line 79 only listsnapari_{419,54}. The header should includenapari_70to match the dependency definitions.🔧 Proposed fix
-[testenv:py{310,311,312,313,314,315}-{PyQt5,PySide2,PyQt6,PySide6}-napari_{419,54}] +[testenv:py{310,311,312,313,314,315}-{PyQt5,PySide2,PyQt6,PySide6}-napari_{419,54,70}] deps = {[testenv]deps} napari_419: napari==0.4.19.post1 napari_54: napari==0.5.4 napari_70: napari==0.7.0🤖 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 `@tox.ini` around lines 79 - 84, The testenv header named "py{310,311,312,313,314,315}-{PyQt5,PySide2,PyQt6,PySide6}-napari_{419,54}" is missing the napari_70 variant; update the testenv header to include napari_70 so it matches the deps block (which defines napari_419, napari_54, and napari_70). Locate the testenv header string (the one containing "py{310,311,312,313,314,315}-{PyQt5,PySide2,PyQt6,PySide6}-napari_{419,54}") and add ",70" or ",_70" to the napari brace group to produce "napari_{419,54,70}" (or equivalent consistent naming) so the header aligns with the napari_70 dependency entry.
🤖 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 `@tox.ini`:
- Line 7: The envlist in tox.ini has a typo `py{310,311,312,313.314}` causing a
syntax error; edit the envlist entry (the envlist variable in tox.ini) to
replace the period with a comma so it reads `py{310,311,312,313,314}` (ensure
all other grouped env braces follow the same comma-separated format).
---
Outside diff comments:
In `@tox.ini`:
- Around line 12-13: Remove the obsolete Python 3.9 mapping from the gh-actions
table in tox.ini by deleting the line "3.9: py39" so the gh-actions mapping
matches the PR's intent to drop Python 3.9 support; ensure only the remaining
mappings (e.g., "3.10: py310") remain and that the gh-actions section syntax
remains valid after removal.
- Around line 79-84: The testenv header named
"py{310,311,312,313,314,315}-{PyQt5,PySide2,PyQt6,PySide6}-napari_{419,54}" is
missing the napari_70 variant; update the testenv header to include napari_70 so
it matches the deps block (which defines napari_419, napari_54, and napari_70).
Locate the testenv header string (the one containing
"py{310,311,312,313,314,315}-{PyQt5,PySide2,PyQt6,PySide6}-napari_{419,54}") and
add ",70" or ",_70" to the napari brace group to produce "napari_{419,54,70}"
(or equivalent consistent naming) so the header aligns with the napari_70
dependency entry.
🪄 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: 25f1a2a4-1495-4189-955a-c85eaafae628
📒 Files selected for processing (3)
.github/workflows/test_napari_widgets.ymlpackage/tests/test_PartSegImage/test_image.pytox.ini
🚧 Files skipped from review as they are similar to previous changes (1)
- package/tests/test_PartSegImage/test_image.py
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)
tox.ini (1)
7-18:⚠️ Potential issue | 🟠 MajorSynchronize
gh-actionsPython mapping withenvlist
tox.inimaps3.15 -> py315in[gh-actions], butenvlistonly expandspy{310,311,312,313,314}(whilefail_on_no_env = Trueis set). This can cause the Python 3.15 job to fail to select any matching tox envs even thoughtestenvtemplates forpy315exist.Suggested fix
-envlist = py{310,311,312,313,314}-{PyQt5,PySide2,PyQt6,PySide6}-all, py{310,311,312,313,314}-{PyQt5,PyQt6}-napari_repo,py{310,311,312,313,314}-{PyQt5,PyQt6,PySide2}-napari_{419,54,70}, py{312,313,314}-PySide6-napari_{419,54,70,repo} +envlist = py{310,311,312,313,314,315}-{PyQt5,PySide2,PyQt6,PySide6}-all, py{310,311,312,313,314,315}-{PyQt5,PyQt6}-napari_repo,py{310,311,312,313,314,315}-{PyQt5,PyQt6,PySide2}-napari_{419,54,70}, py{312,313,314,315}-PySide6-napari_{419,54,70,repo}🤖 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 `@tox.ini` around lines 7 - 18, The GH Actions Python mapping includes 3.15 -> py315 but envlist does not expand py315 (envlist uses py{310,311,312,313,314}), causing no matching tox envs when fail_on_no_env=True; update either the envlist to include py315 (add py{315} into the py{...} expansions or separate py315 entries) or remove the 3.15 -> py315 mapping from the [gh-actions] section so the mapping matches envlist; reference the envlist, the [gh-actions] block, and the py315 identifier when making the change.
🤖 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 `@tox.ini`:
- Around line 7-18: The GH Actions Python mapping includes 3.15 -> py315 but
envlist does not expand py315 (envlist uses py{310,311,312,313,314}), causing no
matching tox envs when fail_on_no_env=True; update either the envlist to include
py315 (add py{315} into the py{...} expansions or separate py315 entries) or
remove the 3.15 -> py315 mapping from the [gh-actions] section so the mapping
matches envlist; reference the envlist, the [gh-actions] block, and the py315
identifier when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9480c3df-a5f1-4b56-8642-2fba7ee1d45e
📒 Files selected for processing (10)
.github/workflows/tests.ymlpackage/PartSeg/common_gui/label_create.pypackage/PartSeg/common_gui/napari_image_view.pypackage/PartSeg/plugins/itk_snap_save/__init__.pypackage/PartSeg/plugins/modeling_save/save_modeling_data.pypackage/PartSeg/plugins/old_partseg/old_partseg.pypackage/PartSegCore/analysis/calculation_plan.pypackage/PartSegImage/image.pypackage/tests/test_PartSeg/test_common_gui.pytox.ini
✅ Files skipped from review due to trivial changes (1)
- package/PartSeg/plugins/old_partseg/old_partseg.py
🚧 Files skipped from review as they are similar to previous changes (7)
- package/PartSeg/plugins/itk_snap_save/init.py
- package/PartSeg/plugins/modeling_save/save_modeling_data.py
- package/PartSegImage/image.py
- package/PartSegCore/analysis/calculation_plan.py
- .github/workflows/tests.yml
- package/PartSeg/common_gui/label_create.py
- package/PartSeg/common_gui/napari_image_view.py
|
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 CodeRabbit
Breaking Changes
Improvements
Refactor
Chores