test: reorganize the suite by module and parallelize with pytest-xdist - #72
Merged
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR expands CI coverage, centralizes test fixtures and integration harnesses, adds MCP and model validation, broadens streaming and data-manager tests, and introduces regression coverage for configuration, checkpoint, packaging, patching, inference, and loss behavior. ChangesTest infrastructure and coverage
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 2
🧹 Nitpick comments (14)
tests/unit/conftest.py (1)
120-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnnotate the shared driver's signature.
_drive_ttais now the cross-module test API (exported via thedrive_ttafixture and used fromtest_streamed_tta.py,test_predictor.py); the untyped params and missing return type make the contract hard to read at call sites.♻️ Suggested annotations
def _drive_tta( - tmp_path, - monkeypatch, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, *, - augmentation, + augmentation: DataAugmentation, streamed: bool, reduction: Reduction | None = None, - transforms=(), - after=(), + transforms: Sequence[Transform] = (), + after: Sequence[Transform] = (), dtype: torch.dtype = torch.float32, - patch_combine=None, + patch_combine: PatchCombine | None = None, case_index: int = 0, file_format: str = "h5", worth_gate: bool = False, -): +) -> tuple[torch.Tensor, 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/conftest.py` around lines 120 - 134, Annotate the shared _drive_tta test driver signature with explicit types for its parameters, including pytest fixtures, augmentation, transforms, reduction, patch_combine, and file_format, and add its return type. Use the existing project types and symbols so the exported drive_tta fixture presents a readable, complete contract to test_streamed_tta.py and test_predictor.py.konfai-mcp/tests/conftest.py (1)
33-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicate
workspace_rootfixtures.
konfai-mcp/tests/test_mcp_server.pyandkonfai-mcp/tests/test_mcp_server_reliability.pystill define same-named local fixtures, which shadow this shared fixture. Delete those copies so all MCP tests use one environment/path setup and cannot drift.🤖 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-mcp/tests/conftest.py` around lines 33 - 38, Remove the duplicate local workspace_root fixtures from test_mcp_server.py and test_mcp_server_reliability.py, leaving the shared tests/conftest.py workspace_root fixture as the single source for environment and path setup. Preserve each test’s existing use of the fixture.tests/unit/test_network.py (2)
563-563: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valuePrefer
weights_only=Truefor the round-trip load.The checkpoint is produced by the test itself, so the flagged deserialization risk is not exploitable here, but the saved payload is plain tensors/ints and loads fine under the safe path — which also keeps the test aligned with the repository's
safe_torch_loadposture.🤖 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_network.py` at line 563, Update the checkpoint load in the round-trip test to pass weights_only=True instead of weights_only=False, preserving the existing CPU map_location and state_dict validation.Source: Linters/SAST tools
499-527: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHoist the nested
Root/Sub/with_optimizersscaffolding into a shared helper.This block is a near-verbatim copy of lines 458-479 in
test_load_restores_nested_network_optimizer_and_counters. The two tests must agree on the nested layout for the key-convention contract to be meaningful, so keeping one definition prevents silent drift.♻️ Sketch
+def _nested_root_with_optimizers() -> tuple[Network, Network]: + """A Root(Network) holding a Sub(Network) with a real AdamW, plus that Sub.""" + ... + def test_checkpoint_save_round_trip_restores_nested_network_state(tmp_path: Path, monkeypatch) -> None: - class Sub(Network): - ... - source = with_optimizers(Root()) - saved_sub = next(net for name, net in source.get_networks().items() if name.endswith(".Sub")) + source, saved_sub = _nested_root_with_optimizers()🤖 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_network.py` around lines 499 - 527, Extract the duplicated nested Root/Sub network setup and optimizer-assignment logic from test_checkpoint_save_round_trip_restores_nested_network_state and test_load_restores_nested_network_optimizer_and_counters into one shared test helper. Update both tests to use that helper while preserving the identical nested layout and optimizer behavior required by the checkpoint key contract.tests/unit/test_transform.py (1)
471-490: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIsolate the process environment:
infer_entrymutatesos.environfor real.
infer_entrydoesos.environ.setdefault("PYTORCH_CUDA_ALLOC_CONF", ...)and popsKONFAI_MASTER_PORT/KONFAI_TENSORBOARD_PORT. This test never registers those keys withmonkeypatch, so the values it leaves behind persist for the rest of the worker's session (and make the allocator test at Line 493 order-dependent).♻️ Suggested guard
monkeypatch.setattr(konfai_apps, "KonfAIApp", FakeKonfAIApp) + for name in ("PYTORCH_CUDA_ALLOC_CONF", "KONFAI_MASTER_PORT", "KONFAI_TENSORBOARD_PORT"): + monkeypatch.delenv(name, raising=False) overrides = ["iterations=300"]🤖 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_transform.py` around lines 471 - 490, Update test_konfai_inference_forwards_config_overrides_to_the_nested_run to isolate environment changes made by infer_entry: register or otherwise monkeypatch PYTORCH_CUDA_ALLOC_CONF, KONFAI_MASTER_PORT, and KONFAI_TENSORBOARD_PORT before invoking the transform, so the test restores their original state automatically while preserving the existing assertions.konfai-mcp/tests/test_mcp_server_dataset_tools.py (1)
90-94: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the metrics-JSON fixture into
mcp_test_helpers. Both modules hand-roll the same evaluation-JSON payload, and the two_metric_jsonhelpers take their arguments in opposite orders, which is an easy trap when this code is copied into a third module.
konfai-mcp/tests/test_mcp_server_dataset_tools.py#L90-L94: keep_write_run_metricsbut have it delegate to a single sharedmetric_json(metric_name, value)exported frommcp_test_helpers.konfai-mcp/tests/test_mcp_server.py#L78-L84: drop the local_metric_jsonand import the shared helper.🤖 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-mcp/tests/test_mcp_server_dataset_tools.py` around lines 90 - 94, Move the shared metrics-JSON fixture into mcp_test_helpers as an exported metric_json(metric_name, value) helper. In konfai-mcp/tests/test_mcp_server_dataset_tools.py:90-94, keep _write_run_metrics but delegate payload creation to metric_json using that argument order; in konfai-mcp/tests/test_mcp_server.py:78-84, remove the local _metric_json and import the shared helper.konfai-mcp/tests/test_mcp_server_reliability.py (1)
163-204: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
tmp_pathinstead of hardcoded/tmppaths.
cwd,log_path, andconfig_pathare hardcoded to/tmp//tmp/job.log//tmp/Config.ymlfor both theactivejob and the conflictinglaunch()call. This PR's own goal is per-test sentinel paths to avoid file sharing across pytest-xdist workers; a fixed literal path here is inconsistent with that (and with thetmp_path-based pattern used in sibling tests, e.g.test_mcp_server_tool_index.py). Static analysis also flags this pattern (CWE-377).♻️ Suggested fix
-def test_device_scoped_job_concurrency() -> None: +def test_device_scoped_job_concurrency(tmp_path: Path) -> None: from konfai_mcp.server_jobs import Job, JobRegistry registry = JobRegistry({"queued", "running"}) active = Job( job_id="gpu0", session="default", kind="train", command=["fake"], - cwd=Path("/tmp"), - log_path=Path("/tmp/job.log"), - config_path=Path("/tmp/Config.yml"), + cwd=tmp_path, + log_path=tmp_path / "job.log", + config_path=tmp_path / "Config.yml", status="running", devices=["0"], ) @@ registry.launch( session="default", kind="train", command=["fake"], - cwd=Path("/tmp"), - log_path=Path("/tmp/job2.log"), - config_path=Path("/tmp/Config.yml"), + cwd=tmp_path, + log_path=tmp_path / "job2.log", + config_path=tmp_path / "Config.yml", devices=["0"], target="mcp_test_helpers:_fake_job_runtime", )🤖 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-mcp/tests/test_mcp_server_reliability.py` around lines 163 - 204, Update test_device_scoped_job_concurrency to accept pytest’s tmp_path fixture and derive cwd, log_path, and config_path from it for both the active Job and registry.launch call. Replace every hardcoded /tmp path with distinct paths under tmp_path, preserving the existing device-conflict assertions.Source: Linters/SAST tools
tests/unit/model_oracles.py (1)
65-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
oracle.eval()inside the helper for symmetry withparametric_leaves_in_execution_order.
parametric_leaves_in_execution_order(Line 101) puts the model in eval mode itself, while this helper silently depends on the caller having done so; a future caller that forgets it gets train-mode norms and a confusing tolerance failure instead of a clear error.♻️ Proposed tweak
captured: dict[int, torch.Tensor] = {} + oracle.eval() handles = [🤖 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/model_oracles.py` around lines 65 - 86, Update capture_oracle_seg_outputs to put oracle in evaluation mode before running the hooked forward pass, matching the behavior of parametric_leaves_in_execution_order and ensuring captured outputs are produced with evaluation-mode layers.tests/unit/test_models_parametric.py (3)
241-264: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
in_channelsandstridesare unused by_expected_resenc_leaf_count.Neither parameter is referenced in the body (the docstring already explains why a stride-only skip contributes nothing), so they only add noise at the call site on Line 311.
♻️ Proposed cleanup
def _expected_resenc_leaf_count( - in_channels: int, n_stages: int, features: list, - strides: list, n_blocks: object, n_conv_decoder: object, ) -> int:assert transferred == _expected_resenc_leaf_count(n_stages, features, n_blocks, n_conv_decoder)🤖 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_models_parametric.py` around lines 241 - 264, Remove the unused in_channels and strides parameters from _expected_resenc_leaf_count, update its signature and all call sites to pass only n_stages, features, n_blocks, and n_conv_decoder, and preserve the existing leaf-count calculation.
45-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMark only the oracle-backed tests
slow
pytestmark = pytest.mark.slowfilters out the oracle-free structural checks too, sopixi run test-fastskips them. Split the module or applyslowonly to the oracle-backed tests.🤖 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_models_parametric.py` at line 45, Remove the module-wide pytestmark from tests in test_models_parametric.py and apply pytest.mark.slow only to tests that use the oracle; leave oracle-free structural checks unmarked so test-fast still runs them.
363-367: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid hard-coding the oracle’s
state_dict()size here.
dynamic_network_architecturesisn’t pinned in this repo, so the572check is tied to the installed nnU-Net implementation rather than KonfAI’s own invariant. Pin that dependency in the test environment, or drop the exact tensor-count assertion and keep the local parameter check instead.🤖 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_models_parametric.py` around lines 363 - 367, Remove the exact len(oracle_ds.state_dict()) == 572 assertion from the test, while retaining the local net parameter-count check and the comparison against oracle_ds.parameters(). Do not add a dependency pin; keep the test focused on KonfAI’s parameter-count invariant.tests/unit/test_models_yaml.py (3)
319-350: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnnotate the parametrized arguments of the three
SEGRESNET_DIM_CASEStests.
dim,upsample_mode,input_shapeandparam_countare unannotated on Lines 320, 335 and 350, unlike the rest of the module (e.g. Line 751). As per coding guidelines, "New public functions require type annotations".♻️ Proposed signature
-def test_segresnet_builds_with_expected_parameter_count(dim, upsample_mode, input_shape, param_count) -> None: +def test_segresnet_builds_with_expected_parameter_count( + dim: int, upsample_mode: str, input_shape: tuple[int, ...], param_count: int +) -> 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 `@tests/unit/test_models_yaml.py` around lines 319 - 350, Annotate the parametrized arguments in test_segresnet_builds_with_expected_parameter_count, test_segresnet_forward_preserves_spatial_and_channels, and test_weight_exact_vs_monai_segresnet with the appropriate types, matching the annotation style already used elsewhere in the module.Source: Coding guidelines
42-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo leaf-ordering helpers are now used side by side in this module.
parametric_leaves_in_execution_orderis imported frommodel_oracles(used at Lines 1456-1457), while the ViT test at Lines 1311-1314 still calls the private_parametric_leaves_in_execution_orderfromkonfai.utils.pretrained. Picking one — the shared oracle helper, so the test does not depend on a private symbol of the code under test — keeps the two from drifting.🤖 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_models_yaml.py` around lines 42 - 53, Update the ViT test to use the imported parametric_leaves_in_execution_order helper from model_oracles instead of the private _parametric_leaves_in_execution_order symbol from konfai.utils.pretrained, and remove the private import if it is no longer used.
1655-1668: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep the Stem test explicit about the catalog defaults The test still depends on the YAML’s
in_channels=5andnum_classes=12defaults while only overridingkernel_sizeandnegative_slope. Passing those values explicitly would make the intent clearer and keep the failure signal focused on the stem knobs.🤖 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_models_yaml.py` around lines 1655 - 1668, Update the build_model_from_yaml call in the Stem test to explicitly include the catalog-default in_channels=5 and num_classes=12 alongside kernel_size and negative_slope. Keep the existing shape and LeakyReLU assertions unchanged.
🤖 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/konfai_studio_ci.yml:
- Around line 27-30: Update the actions/checkout step in the workflow to set
persist-credentials to false while retaining fetch-depth: 0 for tag
reachability.
In `@konfai-mcp/tests/test_mcp_server.py`:
- Around line 348-353: Split the combined pytest.raises block around the two
client.call_tool invocations: assert the expected Trainer-root-key exception
from write_session_file separately, then invoke run_train for Bad.yml in its own
expectation so that validation path executes. Preserve the existing exception
message matching.
---
Nitpick comments:
In `@konfai-mcp/tests/conftest.py`:
- Around line 33-38: Remove the duplicate local workspace_root fixtures from
test_mcp_server.py and test_mcp_server_reliability.py, leaving the shared
tests/conftest.py workspace_root fixture as the single source for environment
and path setup. Preserve each test’s existing use of the fixture.
In `@konfai-mcp/tests/test_mcp_server_dataset_tools.py`:
- Around line 90-94: Move the shared metrics-JSON fixture into mcp_test_helpers
as an exported metric_json(metric_name, value) helper. In
konfai-mcp/tests/test_mcp_server_dataset_tools.py:90-94, keep _write_run_metrics
but delegate payload creation to metric_json using that argument order; in
konfai-mcp/tests/test_mcp_server.py:78-84, remove the local _metric_json and
import the shared helper.
In `@konfai-mcp/tests/test_mcp_server_reliability.py`:
- Around line 163-204: Update test_device_scoped_job_concurrency to accept
pytest’s tmp_path fixture and derive cwd, log_path, and config_path from it for
both the active Job and registry.launch call. Replace every hardcoded /tmp path
with distinct paths under tmp_path, preserving the existing device-conflict
assertions.
In `@tests/unit/conftest.py`:
- Around line 120-134: Annotate the shared _drive_tta test driver signature with
explicit types for its parameters, including pytest fixtures, augmentation,
transforms, reduction, patch_combine, and file_format, and add its return type.
Use the existing project types and symbols so the exported drive_tta fixture
presents a readable, complete contract to test_streamed_tta.py and
test_predictor.py.
In `@tests/unit/model_oracles.py`:
- Around line 65-86: Update capture_oracle_seg_outputs to put oracle in
evaluation mode before running the hooked forward pass, matching the behavior of
parametric_leaves_in_execution_order and ensuring captured outputs are produced
with evaluation-mode layers.
In `@tests/unit/test_models_parametric.py`:
- Around line 241-264: Remove the unused in_channels and strides parameters from
_expected_resenc_leaf_count, update its signature and all call sites to pass
only n_stages, features, n_blocks, and n_conv_decoder, and preserve the existing
leaf-count calculation.
- Line 45: Remove the module-wide pytestmark from tests in
test_models_parametric.py and apply pytest.mark.slow only to tests that use the
oracle; leave oracle-free structural checks unmarked so test-fast still runs
them.
- Around line 363-367: Remove the exact len(oracle_ds.state_dict()) == 572
assertion from the test, while retaining the local net parameter-count check and
the comparison against oracle_ds.parameters(). Do not add a dependency pin; keep
the test focused on KonfAI’s parameter-count invariant.
In `@tests/unit/test_models_yaml.py`:
- Around line 319-350: Annotate the parametrized arguments in
test_segresnet_builds_with_expected_parameter_count,
test_segresnet_forward_preserves_spatial_and_channels, and
test_weight_exact_vs_monai_segresnet with the appropriate types, matching the
annotation style already used elsewhere in the module.
- Around line 42-53: Update the ViT test to use the imported
parametric_leaves_in_execution_order helper from model_oracles instead of the
private _parametric_leaves_in_execution_order symbol from
konfai.utils.pretrained, and remove the private import if it is no longer used.
- Around line 1655-1668: Update the build_model_from_yaml call in the Stem test
to explicitly include the catalog-default in_channels=5 and num_classes=12
alongside kernel_size and negative_slope. Keep the existing shape and LeakyReLU
assertions unchanged.
In `@tests/unit/test_network.py`:
- Line 563: Update the checkpoint load in the round-trip test to pass
weights_only=True instead of weights_only=False, preserving the existing CPU
map_location and state_dict validation.
- Around line 499-527: Extract the duplicated nested Root/Sub network setup and
optimizer-assignment logic from
test_checkpoint_save_round_trip_restores_nested_network_state and
test_load_restores_nested_network_optimizer_and_counters into one shared test
helper. Update both tests to use that helper while preserving the identical
nested layout and optimizer behavior required by the checkpoint key contract.
In `@tests/unit/test_transform.py`:
- Around line 471-490: Update
test_konfai_inference_forwards_config_overrides_to_the_nested_run to isolate
environment changes made by infer_entry: register or otherwise monkeypatch
PYTORCH_CUDA_ALLOC_CONF, KONFAI_MASTER_PORT, and KONFAI_TENSORBOARD_PORT before
invoking the transform, so the test restores their original state automatically
while preserving the existing assertions.
🪄 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: ccf9672a-ad3c-4b91-a243-2d7a47f09a9f
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (69)
.github/workflows/konfai_ci.yml.github/workflows/konfai_studio_ci.yml.github/workflows/publish.ymlAGENTS.mdkonfai-apps/tests/integration/test_app_server_http.pykonfai-apps/tests/integration/test_konfai_app_client_remote.pykonfai-apps/tests/integration/test_konfai_apps.pykonfai-apps/tests/unit/test_app_server_helpers.pykonfai-apps/tests/unit/test_finetune_lr_override.pykonfai-apps/tests/unit/test_repo_apps_allowlist.pykonfai-apps/tests/unit/test_upload_basename_collision.pykonfai-mcp/tests/conftest.pykonfai-mcp/tests/test_mcp_server.pykonfai-mcp/tests/test_mcp_server_apps.pykonfai-mcp/tests/test_mcp_server_batch_and_preview.pykonfai-mcp/tests/test_mcp_server_capabilities.pykonfai-mcp/tests/test_mcp_server_cli.pykonfai-mcp/tests/test_mcp_server_dataset_tools.pykonfai-mcp/tests/test_mcp_server_delete_run.pykonfai-mcp/tests/test_mcp_server_experiments.pykonfai-mcp/tests/test_mcp_server_extensions.pykonfai-mcp/tests/test_mcp_server_fixes.pykonfai-mcp/tests/test_mcp_server_jobs.pykonfai-mcp/tests/test_mcp_server_lot4.pykonfai-mcp/tests/test_mcp_server_lot5.pykonfai-mcp/tests/test_mcp_server_pipeline.pykonfai-mcp/tests/test_mcp_server_reliability.pykonfai-mcp/tests/test_mcp_server_run_records.pykonfai-mcp/tests/test_mcp_server_segmentation_pipeline.pykonfai-mcp/tests/test_mcp_server_set_live_tunables.pykonfai-mcp/tests/test_mcp_server_support.pykonfai-mcp/tests/test_mcp_server_tool_index.pykonfai-mcp/tests/test_review_mcp_apps_misc.pykonfai-mcp/tests/test_runner.pypyproject.tomltests/README.mdtests/conftest.pytests/integration/conftest.pytests/integration/harness.pytests/integration/test_konfai_auto_patch_prediction.pytests/integration/test_konfai_auto_patch_training.pytests/integration/test_konfai_core_workflows.pytests/integration/test_konfai_ensemble_tta.pytests/integration/test_konfai_resume.pytests/integration/test_konfai_streamed_evaluation.pytests/integration/test_konfai_streamed_prediction.pytests/unit/conftest.pytests/unit/model_oracles.pytests/unit/test_auto_patching.pytests/unit/test_config.pytests/unit/test_data_manager.pytests/unit/test_dataset_streaming.pytests/unit/test_models_parametric.pytests/unit/test_models_yaml.pytests/unit/test_network.pytests/unit/test_packaging.pytests/unit/test_patching.pytests/unit/test_plainconvunet_parametric.pytests/unit/test_predictor.pytests/unit/test_residualencoderunet_parametric.pytests/unit/test_streamed_read_dispatcher.pytests/unit/test_streamed_tta.pytests/unit/test_streaming_regressions.pytests/unit/test_transform.pytests/unit/test_unetplusplus_parametric.pytests/unit/test_unetplusplus_yaml.pytests/unit/test_yaml_model_catalog.pytests/unit/test_yaml_model_plainconvunet.pytests/unit/test_yaml_model_segresnet.py
💤 Files with no reviewable changes (11)
- tests/unit/test_unetplusplus_parametric.py
- konfai-mcp/tests/test_review_mcp_apps_misc.py
- tests/unit/test_yaml_model_segresnet.py
- tests/unit/test_streaming_regressions.py
- konfai-mcp/tests/test_mcp_server_lot4.py
- konfai-mcp/tests/test_mcp_server_fixes.py
- tests/unit/test_unetplusplus_yaml.py
- tests/unit/test_plainconvunet_parametric.py
- tests/unit/test_residualencoderunet_parametric.py
- konfai-mcp/tests/test_mcp_server_lot5.py
- tests/unit/test_yaml_model_plainconvunet.py
vboussot
force-pushed
the
refactor/tests-by-module
branch
from
July 29, 2026 16:15
6e83f3e to
a44d440
Compare
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
tests/unit/test_models_yaml.py (2)
1405-1405: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueResidual
torch.randin/torch.randninputs in the VNet and ResEnc sections.These four sites still build inputs with
torch.randnwhile the rest of the file standardized onseeded_input. The assertions are shape-only or same-x-both-sides, so they are correct, but a failure here isn't reproducible.♻️ Suggested consistency tweak
- x = torch.randn(1, 1, 16, 16, 16) + x = seeded_input(1, 1, 16, 16, 16)- x = torch.randn(1, 1, 16, 16) + x = seeded_input(1, 1, 16, 16)- out = net.forward_tensor(torch.randn(1, RESENC_IN_CHANNELS, 128, 128)) + out = net.forward_tensor(seeded_input(1, RESENC_IN_CHANNELS, 128, 128))Also applies to: 1419-1419, 1452-1452, 1661-1661
🤖 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_models_yaml.py` at line 1405, Replace the remaining torch.randn inputs in the VNet and ResEnc test sections with the existing seeded_input helper, including the sites around lines 1405, 1419, 1452, and 1661. Preserve each test’s current tensor shape and usage so failures become reproducible without changing assertions.
1714-1780: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the hard-coded
85with a named constant.
85is a magic number here; a constant alongsideUNETPP_EXPECTED_WEIGHTED_LEAVESwould make the resnet18 expectation easier to read.🤖 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_models_yaml.py` around lines 1714 - 1780, Replace the hard-coded 85 assertion in test_unetplusplus_yaml_is_forward_exact_resnet18 with a named constant declared alongside UNETPP_EXPECTED_WEIGHTED_LEAVES. Use that constant for the expected transferred weighted-leaf count while preserving the existing resnet18 expectation.tests/unit/test_network.py (2)
570-570: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComparing optimizer
statedicts with==is fragile.Dict equality recurses into tensor values, and
bool(tensor)only works because every parameter here holds a single element. Widening the fixture model (e.g. more channels) would make this assertion raise "Boolean value of Tensor with more than one element is ambiguous" instead of failing meaningfully. Compare entries explicitly withtorch.testing.assert_close.🤖 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_network.py` at line 570, Update the optimizer state comparison in the affected test to compare state-dict entries explicitly rather than using dictionary equality. Iterate through matching entries and use torch.testing.assert_close for tensor values, preserving meaningful validation if fixture tensors contain multiple elements.
505-519: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Sub/Root/with_optimizersare duplicated verbatim from the test above (Lines 458-472).Hoisting them to module scope (or a small factory returning
with_optimizers(Root())) keeps the two checkpoint tests in sync when the fixture model changes.🤖 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_network.py` around lines 505 - 519, Remove the duplicate Sub, Root, and with_optimizers definitions from this checkpoint test and reuse the shared module-scope definitions already declared above. Update both checkpoint tests to construct the model through the shared fixture or factory so future model changes remain synchronized.tests/unit/test_packaging.py (1)
216-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe build writes artifacts into the repo working tree.
--outdironly redirects the wheel; withcwd=_REPO_ROOTand--no-isolation, setuptools still createsbuild/and*.egg-infoin the repo root. With the suite now running underpytest-xdist, that shared mutable state can collide with other tests reading the source tree (and it dirties local checkouts). Consider building from a copy of the tree, or passing a scratch build dir / cleaning up afterwards.🤖 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_packaging.py` around lines 216 - 225, Update the wheel-build test around the subprocess.run invocation to prevent setuptools artifacts such as build/ and *.egg-info from being created in _REPO_ROOT. Run the build from an isolated temporary copy or configure a scratch build directory, while preserving the existing wheel output and return-code assertions.konfai-mcp/tests/test_mcp_server_reliability.py (1)
170-204: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
tmp_pathover hardcoded/tmppaths.
launchis expected to raise before spawning, but if the device-conflict guard ever regresses this test would write/tmp/job2.logon the host (and it is not portable to Windows runners). Takingtmp_path: Pathand derivingcwd/log_path/config_pathfrom it keeps the test hermetic.♻️ Proposed change
-def test_device_scoped_job_concurrency() -> None: +def test_device_scoped_job_concurrency(tmp_path: Path) -> None: from konfai_mcp.server_jobs import Job, JobRegistry registry = JobRegistry({"queued", "running"}) active = Job( job_id="gpu0", session="default", kind="train", command=["fake"], - cwd=Path("/tmp"), - log_path=Path("/tmp/job.log"), - config_path=Path("/tmp/Config.yml"), + cwd=tmp_path, + log_path=tmp_path / "job.log", + config_path=tmp_path / "Config.yml", status="running", devices=["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 `@konfai-mcp/tests/test_mcp_server_reliability.py` around lines 170 - 204, Update test_device_scoped_job_concurrency to accept pytest’s tmp_path fixture and derive cwd, log_path, and config_path from it instead of hardcoded /tmp paths. Keep the existing device-conflict assertions and launch behavior unchanged.Source: Linters/SAST tools
konfai-mcp/tests/test_mcp_server.py (1)
78-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider hoisting
_metric_jsonintomcp_test_helpers.A near-identical metric-JSON builder now exists in
konfai-mcp/tests/test_mcp_server_run_records.py(dict-of-cases variant). One shared helper takingdict[str, float]would cover both call sites and keep the fixture shape in one place.🤖 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-mcp/tests/test_mcp_server.py` around lines 78 - 84, The metric JSON construction is duplicated between `_metric_json` and the corresponding helper in test_mcp_server_run_records.py. Move the shared builder into `mcp_test_helpers` with a `dict[str, float]` input supporting multiple cases, then update both call sites to use it and remove the local helper implementations while preserving the existing fixture shape.
🤖 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 `@tests/integration/test_konfai_streamed_evaluation.py`:
- Around line 33-37: Move the SimpleITK importorskip guard before the from
harness import subprocess_env, write_image statement in the test module,
ensuring the dependency check runs before harness is imported and skips cleanly
when SimpleITK is unavailable.
In `@tests/unit/test_transform.py`:
- Around line 486-490: Update both infer_entry tests to pre-register
KONFAI_MASTER_PORT and KONFAI_TENSORBOARD_PORT with pytest’s monkeypatch before
invoking KonfAIInference.infer_entry, preserving any ambient values so teardown
restores them. Use the existing monkeypatch fixture and leave the assertions
unchanged.
---
Nitpick comments:
In `@konfai-mcp/tests/test_mcp_server_reliability.py`:
- Around line 170-204: Update test_device_scoped_job_concurrency to accept
pytest’s tmp_path fixture and derive cwd, log_path, and config_path from it
instead of hardcoded /tmp paths. Keep the existing device-conflict assertions
and launch behavior unchanged.
In `@konfai-mcp/tests/test_mcp_server.py`:
- Around line 78-84: The metric JSON construction is duplicated between
`_metric_json` and the corresponding helper in test_mcp_server_run_records.py.
Move the shared builder into `mcp_test_helpers` with a `dict[str, float]` input
supporting multiple cases, then update both call sites to use it and remove the
local helper implementations while preserving the existing fixture shape.
In `@tests/unit/test_models_yaml.py`:
- Line 1405: Replace the remaining torch.randn inputs in the VNet and ResEnc
test sections with the existing seeded_input helper, including the sites around
lines 1405, 1419, 1452, and 1661. Preserve each test’s current tensor shape and
usage so failures become reproducible without changing assertions.
- Around line 1714-1780: Replace the hard-coded 85 assertion in
test_unetplusplus_yaml_is_forward_exact_resnet18 with a named constant declared
alongside UNETPP_EXPECTED_WEIGHTED_LEAVES. Use that constant for the expected
transferred weighted-leaf count while preserving the existing resnet18
expectation.
In `@tests/unit/test_network.py`:
- Line 570: Update the optimizer state comparison in the affected test to
compare state-dict entries explicitly rather than using dictionary equality.
Iterate through matching entries and use torch.testing.assert_close for tensor
values, preserving meaningful validation if fixture tensors contain multiple
elements.
- Around line 505-519: Remove the duplicate Sub, Root, and with_optimizers
definitions from this checkpoint test and reuse the shared module-scope
definitions already declared above. Update both checkpoint tests to construct
the model through the shared fixture or factory so future model changes remain
synchronized.
In `@tests/unit/test_packaging.py`:
- Around line 216-225: Update the wheel-build test around the subprocess.run
invocation to prevent setuptools artifacts such as build/ and *.egg-info from
being created in _REPO_ROOT. Run the build from an isolated temporary copy or
configure a scratch build directory, while preserving the existing wheel output
and return-code assertions.
🪄 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: f494b0d8-d9b0-4821-ad11-38ee6d87f646
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (69)
.github/workflows/konfai_ci.yml.github/workflows/konfai_studio_ci.yml.github/workflows/publish.ymlAGENTS.mdkonfai-apps/tests/integration/test_app_server_http.pykonfai-apps/tests/integration/test_konfai_app_client_remote.pykonfai-apps/tests/integration/test_konfai_apps.pykonfai-apps/tests/unit/test_app_server_helpers.pykonfai-apps/tests/unit/test_finetune_lr_override.pykonfai-apps/tests/unit/test_repo_apps_allowlist.pykonfai-apps/tests/unit/test_upload_basename_collision.pykonfai-mcp/tests/conftest.pykonfai-mcp/tests/test_mcp_server.pykonfai-mcp/tests/test_mcp_server_apps.pykonfai-mcp/tests/test_mcp_server_batch_and_preview.pykonfai-mcp/tests/test_mcp_server_capabilities.pykonfai-mcp/tests/test_mcp_server_cli.pykonfai-mcp/tests/test_mcp_server_dataset_tools.pykonfai-mcp/tests/test_mcp_server_delete_run.pykonfai-mcp/tests/test_mcp_server_experiments.pykonfai-mcp/tests/test_mcp_server_extensions.pykonfai-mcp/tests/test_mcp_server_fixes.pykonfai-mcp/tests/test_mcp_server_jobs.pykonfai-mcp/tests/test_mcp_server_lot4.pykonfai-mcp/tests/test_mcp_server_lot5.pykonfai-mcp/tests/test_mcp_server_pipeline.pykonfai-mcp/tests/test_mcp_server_reliability.pykonfai-mcp/tests/test_mcp_server_run_records.pykonfai-mcp/tests/test_mcp_server_segmentation_pipeline.pykonfai-mcp/tests/test_mcp_server_set_live_tunables.pykonfai-mcp/tests/test_mcp_server_support.pykonfai-mcp/tests/test_mcp_server_tool_index.pykonfai-mcp/tests/test_review_mcp_apps_misc.pykonfai-mcp/tests/test_runner.pypyproject.tomltests/README.mdtests/conftest.pytests/integration/conftest.pytests/integration/harness.pytests/integration/test_konfai_auto_patch_prediction.pytests/integration/test_konfai_auto_patch_training.pytests/integration/test_konfai_core_workflows.pytests/integration/test_konfai_ensemble_tta.pytests/integration/test_konfai_resume.pytests/integration/test_konfai_streamed_evaluation.pytests/integration/test_konfai_streamed_prediction.pytests/unit/conftest.pytests/unit/model_oracles.pytests/unit/test_auto_patching.pytests/unit/test_config.pytests/unit/test_data_manager.pytests/unit/test_dataset_streaming.pytests/unit/test_models_parametric.pytests/unit/test_models_yaml.pytests/unit/test_network.pytests/unit/test_packaging.pytests/unit/test_patching.pytests/unit/test_plainconvunet_parametric.pytests/unit/test_predictor.pytests/unit/test_residualencoderunet_parametric.pytests/unit/test_streamed_read_dispatcher.pytests/unit/test_streamed_tta.pytests/unit/test_streaming_regressions.pytests/unit/test_transform.pytests/unit/test_unetplusplus_parametric.pytests/unit/test_unetplusplus_yaml.pytests/unit/test_yaml_model_catalog.pytests/unit/test_yaml_model_plainconvunet.pytests/unit/test_yaml_model_segresnet.py
💤 Files with no reviewable changes (11)
- tests/unit/test_yaml_model_plainconvunet.py
- tests/unit/test_streaming_regressions.py
- tests/unit/test_unetplusplus_yaml.py
- konfai-mcp/tests/test_mcp_server_lot5.py
- tests/unit/test_residualencoderunet_parametric.py
- konfai-mcp/tests/test_review_mcp_apps_misc.py
- tests/unit/test_plainconvunet_parametric.py
- konfai-mcp/tests/test_mcp_server_fixes.py
- tests/unit/test_yaml_model_segresnet.py
- tests/unit/test_unetplusplus_parametric.py
- konfai-mcp/tests/test_mcp_server_lot4.py
🚧 Files skipped from review as they are similar to previous changes (46)
- tests/integration/conftest.py
- .github/workflows/konfai_studio_ci.yml
- konfai-mcp/tests/conftest.py
- konfai-apps/tests/integration/test_konfai_app_client_remote.py
- tests/README.md
- konfai-apps/tests/integration/test_konfai_apps.py
- konfai-apps/tests/unit/test_upload_basename_collision.py
- konfai-apps/tests/integration/test_app_server_http.py
- tests/unit/test_patching.py
- konfai-mcp/tests/test_mcp_server_pipeline.py
- konfai-apps/tests/unit/test_app_server_helpers.py
- pyproject.toml
- tests/unit/test_auto_patching.py
- tests/integration/test_konfai_auto_patch_training.py
- konfai-mcp/tests/test_mcp_server_delete_run.py
- konfai-mcp/tests/test_runner.py
- tests/integration/test_konfai_auto_patch_prediction.py
- konfai-mcp/tests/test_mcp_server_segmentation_pipeline.py
- .github/workflows/konfai_ci.yml
- tests/unit/conftest.py
- tests/conftest.py
- tests/unit/test_config.py
- tests/unit/test_models_parametric.py
- konfai-mcp/tests/test_mcp_server_tool_index.py
- konfai-mcp/tests/test_mcp_server_cli.py
- tests/unit/model_oracles.py
- tests/integration/test_konfai_streamed_prediction.py
- konfai-mcp/tests/test_mcp_server_experiments.py
- tests/integration/harness.py
- konfai-mcp/tests/test_mcp_server_set_live_tunables.py
- konfai-apps/tests/unit/test_finetune_lr_override.py
- AGENTS.md
- tests/unit/test_yaml_model_catalog.py
- tests/unit/test_predictor.py
- konfai-mcp/tests/test_mcp_server_batch_and_preview.py
- konfai-mcp/tests/test_mcp_server_apps.py
- konfai-mcp/tests/test_mcp_server_jobs.py
- konfai-mcp/tests/test_mcp_server_extensions.py
- tests/integration/test_konfai_resume.py
- tests/integration/test_konfai_ensemble_tta.py
- tests/unit/test_streamed_read_dispatcher.py
- tests/unit/test_streamed_tta.py
- tests/integration/test_konfai_core_workflows.py
- tests/unit/test_data_manager.py
- konfai-mcp/tests/test_mcp_server_dataset_tools.py
- tests/unit/test_dataset_streaming.py
vboussot
marked this pull request as draft
July 29, 2026 17:01
Dissolve the streaming-regressions and async-writes grab-bags into the files of the modules they pin, split test_dataset_streaming.py into its dataset / data_manager / read-dispatcher parts, and share the streaming stub and TTA driver through tests/unit/conftest.py.
Move the helpers the integration files imported from each other into harness.py, deduplicate replace_once/write_image, and mark test_konfai_streamed_evaluation.py with the integration marker the other six files already carry.
Dissolve the fixes/lot4/lot5/lot6/review files into the feature files they belong to, rename runner_fixes to test_runner.py, add test_mcp_server_run_records.py, and replace 48 copies of the KONFAI_MCP_WORKSPACES_ROOT setenv with a workspace_root fixture. One near-duplicate describe_app test is merged into its twin.
pytestmark cannot prevent module import, so the five files importing fastapi at module level errored at collection instead of skipping; use the importorskip-before-imports pattern already present in test_remote_tunables_contract.py.
Mark the oracle weight-exactness files, the real-training MCP e2e and the heavyweight apps integrations as slow, and add a pixi test-fast task; the fast profile runs 1006 of 1160 core tests in ~1m40 instead of 15m, while CI keeps running everything.
Inline augmentations must disable persistent workers, the config write-back must stay atomic when the rename fails, nested-Network state must survive a real checkpoint_save round-trip (the writer and reader are now exercised against each other), and the empty-mask MaskedLoss test now asserts the NaN its name promises.
Merge the seven per-model oracle files into test_models_parametric.py (Python model vs external oracle) and test_models_yaml.py (catalog entry vs reference), with the shared builders in model_oracles.py. Every node survives except the two default-catalog-entry duplicates already covered by test_every_catalog_entry_builds.
Delete the eight per-transform stream-vs-whole tests whose exact property the enumerated locality contract already proves, and keep the dispatcher-specific ones: region-fold composition, planner classification and state, refusals, and resample region primitives.
Build a wheel and assert it ships every models/python module and catalog yml derived from the source tree, leaks no sibling package, and keeps the PEP 420 namespaces bare; the CI build job delegates to this test instead of its hard-coded heredoc counts.
Studio had no PR workflow and publish.yml built the React front only after the test job, so the served-frontend assertion always skipped; the new workflow and the reordered test job build it first.
State the per-module test layout, marker semantics and env-via- fixtures rule in tests/README.md, point agents at test-fast for iteration, and refresh the stale union-coercion trap entry.
Run pixi test/test-cov with -n auto --dist loadfile (per-file distribution keeps module-scoped fixtures and in-file ordering on one worker) and make the conftest sentinel config path per-test so parallel workers never share a file. Full core suite: 12m21 -> 6m01.
The pytest.raises block held two calls and the exception came from the second: writing the bad config succeeds (validation happens at launch), so the block documented the wrong guard. Split so each expectation stands alone. The studio CI checkout stops persisting credentials -- PR CI runs untrusted build steps and never needs git auth.
The rebase's union merge of test_patching.py bypassed the format hook (rebase runs no pre-commit). The SimpleITK guard moves above the harness import that itself imports SimpleITK, so a bare environment skips instead of erroring. And infer_entry pops both KONFAI port vars from the real environment: an autouse fixture registers them so teardown restores the ambient values.
vboussot
marked this pull request as ready for review
July 29, 2026 17:27
vboussot
force-pushed
the
refactor/tests-by-module
branch
from
July 29, 2026 17:27
6f33a66 to
4497932
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
.github/workflows/publish.yml (1)
50-53: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse a supported Node.js LTS version.
This new step hard-codes Node.js 20, which reached EOL on March 24, 2026. Use the repository’s supported Node.js version (currently Node.js 24 LTS), and update the corresponding Studio packaging step as well so test and wheel builds use the same maintained runtime. (nodejs.org)
🤖 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/publish.yml around lines 50 - 53, Update the “Set up Node” actions/setup-node configuration in the workflow from Node.js 20 to the repository’s supported Node.js 24 LTS version, and update the corresponding Studio packaging setup to use the same runtime.tests/unit/test_network.py (2)
499-499: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnnotate
monkeypatch.-def test_checkpoint_save_round_trip_restores_nested_network_state(tmp_path: Path, monkeypatch) -> None: +def test_checkpoint_save_round_trip_restores_nested_network_state( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None:As per coding guidelines: "New public functions must have type annotations".
🤖 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_network.py` at line 499, Update the test_checkpoint_save_round_trip_restores_nested_network_state function signature to add an appropriate type annotation for the monkeypatch parameter, preserving the existing Path annotation and return type.Source: Coding guidelines
505-526: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Sub/Root/with_optimizersand the save-state setup are copied verbatim fromtest_load_restores_nested_network_optimizer_and_counters.Extracting them into module-level helpers keeps the two tests from drifting (a change to the fixture model would otherwise only be applied to one of them).
🤖 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_network.py` around lines 505 - 526, Extract the duplicated Sub, Root, and with_optimizers definitions plus the shared initialized/save-state setup into module-level test helpers, then update both test_load_restores_nested_network_optimizer_and_counters and this test to reuse them while preserving their existing assertions and state values.konfai-mcp/tests/test_mcp_server_reliability.py (1)
163-204: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
tmp_pathinstead of hardcoded/tmppaths.
Path("/tmp"),/tmp/job.log,/tmp/Config.ymlare never written here (the conflict check raises first), but a future change toJobRegistry.launchthat toucheslog_path/config_pathwould write into a shared, world-writable location and make the test order-dependent.♻️ Proposed tweak
-def test_device_scoped_job_concurrency() -> None: +def test_device_scoped_job_concurrency(tmp_path: Path) -> None: from konfai_mcp.server_jobs import Job, JobRegistry registry = JobRegistry({"queued", "running"}) active = Job( job_id="gpu0", session="default", kind="train", command=["fake"], - cwd=Path("/tmp"), - log_path=Path("/tmp/job.log"), - config_path=Path("/tmp/Config.yml"), + cwd=tmp_path, + log_path=tmp_path / "job.log", + config_path=tmp_path / "Config.yml", status="running", devices=["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 `@konfai-mcp/tests/test_mcp_server_reliability.py` around lines 163 - 204, Update test_device_scoped_job_concurrency to accept pytest’s tmp_path fixture and use it for the active job’s cwd, log_path, and config_path, as well as the launch call’s corresponding paths, instead of hardcoded /tmp locations.Source: Linters/SAST tools
tests/unit/test_packaging.py (1)
207-225: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
python -m buildin_REPO_ROOTwrites into the working tree.Only the wheel lands in
tmp_path; setuptools still createsbuild/and*.egg-infounder the repo root. Underpytest-xdist(or alongside a CI wheel build sharing the checkout) that shared state can collide, and it leaves the tree dirty for subsequent steps. Copying the sources intotmp_pathbefore building, or asserting/cleaning the generated dirs, removes the coupling.🤖 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_packaging.py` around lines 207 - 225, Update test_wheel_ships_model_zoo_and_catalog so the wheel builds from an isolated temporary source tree rather than _REPO_ROOT, preventing build/ and *.egg-info artifacts or concurrent-build collisions in the checkout. Copy the repository sources needed by the build into tmp_path, run the existing python -m build command there, and continue validating the single wheel produced in the temporary output location.
🤖 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-mcp/tests/test_mcp_server_run_records.py`:
- Around line 53-66: Update the test around both wait_for_job calls in the
run-records test to retain each result and assert that the corresponding job
completed successfully before performing record or diff assertions. Validate the
returned job status explicitly, while preserving the existing config and
manifest inspection.
---
Nitpick comments:
In @.github/workflows/publish.yml:
- Around line 50-53: Update the “Set up Node” actions/setup-node configuration
in the workflow from Node.js 20 to the repository’s supported Node.js 24 LTS
version, and update the corresponding Studio packaging setup to use the same
runtime.
In `@konfai-mcp/tests/test_mcp_server_reliability.py`:
- Around line 163-204: Update test_device_scoped_job_concurrency to accept
pytest’s tmp_path fixture and use it for the active job’s cwd, log_path, and
config_path, as well as the launch call’s corresponding paths, instead of
hardcoded /tmp locations.
In `@tests/unit/test_network.py`:
- Line 499: Update the
test_checkpoint_save_round_trip_restores_nested_network_state function signature
to add an appropriate type annotation for the monkeypatch parameter, preserving
the existing Path annotation and return type.
- Around line 505-526: Extract the duplicated Sub, Root, and with_optimizers
definitions plus the shared initialized/save-state setup into module-level test
helpers, then update both
test_load_restores_nested_network_optimizer_and_counters and this test to reuse
them while preserving their existing assertions and state values.
In `@tests/unit/test_packaging.py`:
- Around line 207-225: Update test_wheel_ships_model_zoo_and_catalog so the
wheel builds from an isolated temporary source tree rather than _REPO_ROOT,
preventing build/ and *.egg-info artifacts or concurrent-build collisions in the
checkout. Copy the repository sources needed by the build into tmp_path, run the
existing python -m build command there, and continue validating the single wheel
produced in the temporary output location.
🪄 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: 50faa5e7-61a8-4966-9cb5-93fac056f1fb
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (69)
.github/workflows/konfai_ci.yml.github/workflows/konfai_studio_ci.yml.github/workflows/publish.ymlAGENTS.mdkonfai-apps/tests/integration/test_app_server_http.pykonfai-apps/tests/integration/test_konfai_app_client_remote.pykonfai-apps/tests/integration/test_konfai_apps.pykonfai-apps/tests/unit/test_app_server_helpers.pykonfai-apps/tests/unit/test_finetune_lr_override.pykonfai-apps/tests/unit/test_repo_apps_allowlist.pykonfai-apps/tests/unit/test_upload_basename_collision.pykonfai-mcp/tests/conftest.pykonfai-mcp/tests/test_mcp_server.pykonfai-mcp/tests/test_mcp_server_apps.pykonfai-mcp/tests/test_mcp_server_batch_and_preview.pykonfai-mcp/tests/test_mcp_server_capabilities.pykonfai-mcp/tests/test_mcp_server_cli.pykonfai-mcp/tests/test_mcp_server_dataset_tools.pykonfai-mcp/tests/test_mcp_server_delete_run.pykonfai-mcp/tests/test_mcp_server_experiments.pykonfai-mcp/tests/test_mcp_server_extensions.pykonfai-mcp/tests/test_mcp_server_fixes.pykonfai-mcp/tests/test_mcp_server_jobs.pykonfai-mcp/tests/test_mcp_server_lot4.pykonfai-mcp/tests/test_mcp_server_lot5.pykonfai-mcp/tests/test_mcp_server_pipeline.pykonfai-mcp/tests/test_mcp_server_reliability.pykonfai-mcp/tests/test_mcp_server_run_records.pykonfai-mcp/tests/test_mcp_server_segmentation_pipeline.pykonfai-mcp/tests/test_mcp_server_set_live_tunables.pykonfai-mcp/tests/test_mcp_server_support.pykonfai-mcp/tests/test_mcp_server_tool_index.pykonfai-mcp/tests/test_review_mcp_apps_misc.pykonfai-mcp/tests/test_runner.pypyproject.tomltests/README.mdtests/conftest.pytests/integration/conftest.pytests/integration/harness.pytests/integration/test_konfai_auto_patch_prediction.pytests/integration/test_konfai_auto_patch_training.pytests/integration/test_konfai_core_workflows.pytests/integration/test_konfai_ensemble_tta.pytests/integration/test_konfai_resume.pytests/integration/test_konfai_streamed_evaluation.pytests/integration/test_konfai_streamed_prediction.pytests/unit/conftest.pytests/unit/model_oracles.pytests/unit/test_auto_patching.pytests/unit/test_config.pytests/unit/test_data_manager.pytests/unit/test_dataset_streaming.pytests/unit/test_models_parametric.pytests/unit/test_models_yaml.pytests/unit/test_network.pytests/unit/test_packaging.pytests/unit/test_patching.pytests/unit/test_plainconvunet_parametric.pytests/unit/test_predictor.pytests/unit/test_residualencoderunet_parametric.pytests/unit/test_streamed_read_dispatcher.pytests/unit/test_streamed_tta.pytests/unit/test_streaming_regressions.pytests/unit/test_transform.pytests/unit/test_unetplusplus_parametric.pytests/unit/test_unetplusplus_yaml.pytests/unit/test_yaml_model_catalog.pytests/unit/test_yaml_model_plainconvunet.pytests/unit/test_yaml_model_segresnet.py
💤 Files with no reviewable changes (11)
- tests/unit/test_plainconvunet_parametric.py
- tests/unit/test_yaml_model_plainconvunet.py
- tests/unit/test_unetplusplus_parametric.py
- konfai-mcp/tests/test_mcp_server_lot5.py
- tests/unit/test_unetplusplus_yaml.py
- konfai-mcp/tests/test_mcp_server_fixes.py
- konfai-mcp/tests/test_review_mcp_apps_misc.py
- tests/unit/test_streaming_regressions.py
- tests/unit/test_yaml_model_segresnet.py
- tests/unit/test_residualencoderunet_parametric.py
- konfai-mcp/tests/test_mcp_server_lot4.py
🚧 Files skipped from review as they are similar to previous changes (44)
- .github/workflows/konfai_studio_ci.yml
- tests/README.md
- konfai-mcp/tests/test_mcp_server_pipeline.py
- konfai-apps/tests/unit/test_repo_apps_allowlist.py
- konfai-apps/tests/unit/test_app_server_helpers.py
- konfai-mcp/tests/test_mcp_server_delete_run.py
- tests/unit/test_yaml_model_catalog.py
- tests/integration/test_konfai_auto_patch_training.py
- konfai-mcp/tests/conftest.py
- konfai-mcp/tests/test_mcp_server_segmentation_pipeline.py
- tests/integration/conftest.py
- tests/conftest.py
- konfai-apps/tests/integration/test_app_server_http.py
- tests/unit/test_auto_patching.py
- konfai-apps/tests/unit/test_finetune_lr_override.py
- konfai-mcp/tests/test_mcp_server_support.py
- konfai-mcp/tests/test_mcp_server_cli.py
- konfai-mcp/tests/test_mcp_server_tool_index.py
- konfai-mcp/tests/test_mcp_server_extensions.py
- tests/integration/test_konfai_streamed_prediction.py
- konfai-apps/tests/unit/test_upload_basename_collision.py
- pyproject.toml
- tests/unit/test_patching.py
- .github/workflows/konfai_ci.yml
- konfai-mcp/tests/test_mcp_server_set_live_tunables.py
- tests/integration/test_konfai_streamed_evaluation.py
- AGENTS.md
- tests/integration/test_konfai_resume.py
- tests/integration/harness.py
- tests/unit/conftest.py
- tests/integration/test_konfai_ensemble_tta.py
- konfai-mcp/tests/test_mcp_server_batch_and_preview.py
- tests/unit/model_oracles.py
- tests/unit/test_predictor.py
- tests/unit/test_config.py
- tests/integration/test_konfai_core_workflows.py
- tests/unit/test_dataset_streaming.py
- konfai-mcp/tests/test_mcp_server_jobs.py
- tests/unit/test_models_parametric.py
- tests/unit/test_streamed_tta.py
- tests/unit/test_streamed_read_dispatcher.py
- konfai-mcp/tests/test_mcp_server_dataset_tools.py
- tests/unit/test_data_manager.py
- tests/unit/test_models_yaml.py
The config snapshots and the manifest exist whatever the outcome, so a run ending in error or killed satisfied every assertion that followed.
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.
Test-infrastructure batch, pushed as part of release preparation:
test-fastloopNote for sequencing: this reorganizes
tests/unit, which #71 appends tests to — whichever merges second must rebase (planned: this one, after #71).Summary by CodeRabbit
Chores
Documentation
Tests