fix: audit correctness batch (config binder, nested resume, geometry, metrics) - #60
Merged
Conversation
The union binder tried members in declaration order with bare candidate_type(value), so overlap: 0.25 bound int(0.25) == 0 (silent no overlap), a str member swallowed a list, and a list[...] member (never a type) could not bind at all. Match the value against each member by isinstance first and keep it when its runtime type already fits; fall back to coercion only otherwise.
checkpoint_save writes a nested network's optimizer/iteration/LR-schedule state under its dotted get_networks() key (e.g. Gan.Generator_optimizer_state_dict), but Network.load looked it up under the bare class name, so every composite model (the GAN family) silently resumed with a fresh Adam and _it == 0. load now consumes the dotted key _apply_network already injects. Single-network models are unaffected (their key equals the class name); the weights path is untouched.
ResampleTransform added the stored transform's physical (dx, dy, dz) displacement directly onto a (z, y, x) voxel-index grid, transposing the x/z axes and treating millimetres as voxel counts, so any non-symmetric warp came out geometrically wrong. Apply the transform with sitk.Resample instead, which honours spacing, direction and units by construction (nearest for uint8 labels, linear otherwise). ResampleTransform is not used by any shipped config; a known-translation test pins the corrected axis and magnitude.
FID.preprocess_images called torch.nn.functional.resize / .normalize(mean, std), neither of which exists there, so the metric raised on every use; use torchvision.transforms.functional and follow the input device. Accuracy accumulated n/corrects on the instance for the whole process, reporting one fraction that blended every epoch and both splits; return the current batch's accuracy and let the logging window mean and reset it like every other metric.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request updates SimpleITK resampling, metric calculations, FID preprocessing, nested network checkpoint loading, union-type configuration conversion, and macOS CI networking, with tests covering the behavior changes. ChangesBehavior updates
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant ResampleTransform
participant TransformStore
participant SimpleITK
participant TorchTensor
ResampleTransform->>TransformStore: retrieve result_transform
ResampleTransform->>SimpleITK: convert tensor to image
ResampleTransform->>SimpleITK: resample in physical space
SimpleITK->>TorchTensor: convert image_to_data
ResampleTransform->>TorchTensor: apply output dtype
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/unit/test_config.py (1)
317-343: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the bool-specific union branch.
The test does not exercise the new
boolversusint/floatbehavior. Add a union ordered asint | float | bool, configuretrue, and assert that the bound value remains a realbool.Suggested test addition
write_config("Root:\n frac: 0.25\n voxels: 8\n percent: '20%'\n per_axis:\n - 10\n - 20\n - 0\n+ flag: true\n per_axis: int | float | str | list[int] | None = None, + flag: int | float | bool | None = None, ) -> None: @@ self.per_axis = per_axis + self.flag = flag @@ assert list(root.per_axis) == [10, 20, 0] and isinstance(root.per_axis, list) + assert root.flag is True and isinstance(root.flag, bool)🤖 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 `@tests/unit/test_config.py` around lines 317 - 343, Extend test_apply_config_union_keeps_the_value_type_over_lossy_coercion with a field using the union int | float | bool, configure it with YAML true, and assert the bound value remains True and is exactly a bool rather than being coerced to int or float.
🤖 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 `@konfai/data/transform.py`:
- Around line 1115-1117: Update the conversion logic around image_to_data and
the resulting tensor to preserve the input tensor’s device and dtype: move the
CPU tensor back to tensor.device and retain the original floating dtype instead
of unconditionally converting to float32, while keeping uint8 behavior
unchanged. Add a regression assertion covering dtype and device for a floating
input.
In `@konfai/metric/measure.py`:
- Around line 896-898: Update the FID feature-extraction path around
preprocess_images() and get_features() to ensure preprocessed inputs are moved
to the Inception model’s device before forward(). Reuse the model’s existing
device rather than hardcoding a device, while preserving CPU behavior.
In `@konfai/utils/config.py`:
- Around line 413-416: Update the generic matching branch around get_origin in
the candidate-type resolution logic to avoid unsafe isinstance calls for
typing-only origins and prevent mismatched parameterized containers such as
list[str] versus list[int] from passing. Recursively validate generic arguments
where runtime-checkable, and otherwise skip the fast path so normal type
handling can decide.
---
Nitpick comments:
In `@tests/unit/test_config.py`:
- Around line 317-343: Extend
test_apply_config_union_keeps_the_value_type_over_lossy_coercion with a field
using the union int | float | bool, configure it with YAML true, and assert the
bound value remains True and is exactly a bool rather than being coerced to int
or float.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c7dcb79-404a-4dba-9bd3-062eadc41239
📒 Files selected for processing (8)
konfai/data/transform.pykonfai/metric/measure.pykonfai/network/network.pykonfai/utils/config.pytests/unit/test_config.pytests/unit/test_itk_transforms.pytests/unit/test_measure.pytests/unit/test_network.py
Comment on lines
+1115
to
+1117
| data, _ = image_to_data(resampled) | ||
| result = torch.from_numpy(np.ascontiguousarray(data)) | ||
| return result.to(torch.uint8) if tensor.dtype == torch.uint8 else result.float() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the caller’s device and floating dtype.
torch.from_numpy always creates a CPU tensor, and result.float() converts every non-uint8 input to CPU float32. This regresses CUDA and float64/float16 inputs from the prior Torch path.
Proposed fix
result = torch.from_numpy(np.ascontiguousarray(data))
-return result.to(torch.uint8) if tensor.dtype == torch.uint8 else result.float()
+return result.to(device=tensor.device, dtype=tensor.dtype)Add a regression assertion for a floating input’s dtype and device.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| data, _ = image_to_data(resampled) | |
| result = torch.from_numpy(np.ascontiguousarray(data)) | |
| return result.to(torch.uint8) if tensor.dtype == torch.uint8 else result.float() | |
| data, _ = image_to_data(resampled) | |
| result = torch.from_numpy(np.ascontiguousarray(data)) | |
| return result.to(device=tensor.device, dtype=tensor.dtype) |
🤖 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 `@konfai/data/transform.py` around lines 1115 - 1117, Update the conversion
logic around image_to_data and the resulting tensor to preserve the input
tensor’s device and dtype: move the CPU tensor back to tensor.device and retain
the original floating dtype instead of unconditionally converting to float32,
while keeping uint8 behavior unchanged. Add a regression assertion covering
dtype and device for a floating input.
Comment on lines
+896
to
+898
| tvf = _require_optional("torchvision.transforms.functional", criterion="FID", extra="fid") | ||
| resized = tvf.resize(image, [299, 299]).repeat((1, 3, 1, 1)) | ||
| return tvf.normalize(resized, mean=[0.485, 0.456, 0.406], std=[0.229, 0.224, 0.225]) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map relevant symbols and inspect the surrounding implementation.
ast-grep outline konfai/metric/measure.py --view expanded | sed -n '1,220p'
printf '\n--- FID-related lines ---\n'
rg -n "class FID|def preprocess_images|def forward|get_features|to\\(" konfai/metric/measure.py
printf '\n--- focused excerpt ---\n'
sed -n '820,940p' konfai/metric/measure.pyRepository: fideus-labs/KonfAI
Length of output: 20182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect any tests around FID preprocessing/device handling.
rg -n "FID|preprocess_images|torchvision.transforms.functional|device" tests konfai -g '!**/dist/**' -g '!**/build/**'Repository: fideus-labs/KonfAI
Length of output: 49519
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any explicit device-placement in FID feature extraction.
rg -n "get_features|preprocess_images|cuda|cpu|device" konfai/metric/measure.pyRepository: fideus-labs/KonfAI
Length of output: 4432
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '860,930p' konfai/metric/measure.pyRepository: fideus-labs/KonfAI
Length of output: 3261
Keep FID inputs on the model device
preprocess_images() returns CPU tensors, but FID.__init__() still creates the Inception model on CUDA and get_features() forwards tensors without moving them. That will trip a device mismatch in forward() on GPU. Move the inputs to the model’s device, or make the metric use one device consistently.
🤖 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 `@konfai/metric/measure.py` around lines 896 - 898, Update the FID
feature-extraction path around preprocess_images() and get_features() to ensure
preprocessed inputs are moved to the Inception model’s device before forward().
Reuse the model’s existing device rather than hardcoding a device, while
preserving CPU behavior.
Comment on lines
+413
to
+416
| origin = get_origin(candidate_type) | ||
| if origin is not None: | ||
| if isinstance(value, origin): | ||
| return value |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
python - <<'PY'
from typing import Literal, get_origin
print("list origin match:", isinstance(["x"], get_origin(list[int])))
origin = get_origin(Literal["x"])
try:
isinstance("x", origin)
except TypeError as exc:
print("typing-only origin is not runtime-checkable:", exc)
PYRepository: fideus-labs/KonfAI
Length of output: 273
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
file="konfai/utils/config.py"
# Map the relevant structure first.
ast-grep outline "$file" --view expanded
# Read the surrounding section with line numbers.
sed -n '360,460p' "$file" | cat -nRepository: fideus-labs/KonfAI
Length of output: 5833
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Show where these annotations are accepted and whether Literal is part of the supported binding surface.
rg -n "\bLiteral\b|get_origin\(|_convert_union_sequence_value|Sequence\[|list\[" konfai/utils/config.py konfai -g '!**/__pycache__/**'
# Inspect the call site that supplies valid_types.
sed -n '520,700p' konfai/utils/config.py | cat -nRepository: fideus-labs/KonfAI
Length of output: 50374
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect the binding logic around the union/sequence handling.
sed -n '470,620p' konfai/utils/config.py | cat -n
# Show the helper definitions that feed this branch.
sed -n '385,470p' konfai/utils/config.py | cat -nRepository: fideus-labs/KonfAI
Length of output: 14766
Make generic union-member matching runtime-safe konfai/utils/config.py:413-416
get_origin() only checks the outer container, so list[str] and list[int] both pass this fast path, and typing-only origins like Literal can still raise TypeError in isinstance. Recursively check generic arguments or skip this path unless the origin is runtime-checkable.
🤖 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 `@konfai/utils/config.py` around lines 413 - 416, Update the generic matching
branch around get_origin in the candidate-type resolution logic to avoid unsafe
isinstance calls for typing-only origins and prevent mismatched parameterized
containers such as list[str] versus list[int] from passing. Recursively validate
generic arguments where runtime-checkable, and otherwise skip the fast path so
normal type handling can decide.
The streamed-prediction DDP tests init a gloo process group; on the macOS runners gloo resolves the host's '.local' name and the rendezvous hangs until timeout (a recurring flake, no code fault). A macOS-only step pins GLOO_SOCKET_IFNAME=lo0 so gloo binds the loopback directly; Linux and Windows runners are untouched (the step is skipped there).
This was referenced Aug 3, 2026
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.
Applies the clean, tested, low-risk correctness findings from the audit. Each fix is minimal, guarded by a regression test, and verified not to change the common paths. Riskier or unverifiable findings are intentionally left for dedicated PRs (listed below).
Fixes
candidate_type(value), sooverlap: 0.25boundint(0.25) == 0(silent no overlap → seam artifacts), astrmember swallowed a list, and alist[...]member never bound. Now the value is kept when its runtime type already satisfies a member (isinstance-first), falling back to coercion only otherwise. (P0)checkpoint_savewrites a nested network's optimizer/_it/_nb_lr_updateunder its dottedget_networks()key, butNetwork.loadlooked them up under the bare class name, so every composite model (GAN family) silently resumed with a fresh Adam and_it == 0.loadnow consumes the dotted key_apply_networkalready injects. Single-network models are unaffected (key == class name); the weights path is untouched. (P1)ResampleTransformadded the stored transform's physical(dx,dy,dz)displacement straight onto a(z,y,x)voxel-index grid (x/z transposed, mm treated as voxels). It now resamples viasitk.Resample, honouring spacing/direction/units by construction. Not used by any shipped config. (P1)FID.preprocess_imagescalledtorch.nn.functional.resize/.normalize(mean,std)(neither exists), so FID could never run → usestorchvision.transforms.functional.Accuracyaccumulatedn/correctsfor the whole process (one fraction blending every epoch and both splits) → returns the current batch, letting the logging window mean and reset it. Both were unused in tests/configs. (P2)Tests
New regression tests: union type-preservation (
test_config), nested-composite resume of optimizer/counters (test_network), known-translation geometry (test_itk_transforms), and metric smoke tests for FID + Accuracy (test_measure).Verification
pixi run check(lint + format + core + apps): green ·konfai-mcp: 145 passedIntentionally deferred (own verified PRs — still tracked in the audit)
all_gatheron unequal shards) — needs a 2-GPU reproPerceptualLossdropping losses when targets < losses;LPIPS/IMPACTReghard-codedcuda:0Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores