feat(api): the workflows as Python callables - #94
Conversation
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Comment |
|
@coderabbitai review |
|
ba53ab5 to
a8bbe32
Compare
a8bbe32 to
b822331
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (6)
konfai/data/reduction.py (1)
165-166: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDeclare
working_multiple = 2.0forStd.Stdretains_meanand_m2in addition to the incoming region. The planner therefore needs four regions, including the output, instead of two.🤖 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/reduction.py` around lines 165 - 166, Declare working_multiple = 2.0 alongside voxel_local and incremental for the Std reduction configuration, so the planner allocates four regions including the output.tests/unit/test_api.py (3)
40-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the recording tests to
tests/unit/test_config.pyas well.
record_given_argumentsis new inkonfai/utils/config.pyand changes how constructor arguments bind to the config tree. The behaviour is pinned here only. The guideline requires configuration-binding changes to updatetests/unit/test_config.py.Cases worth pinning there directly: a
**kwargsconstructor (flattened into the record), a*argsconstructor (recordsNone), and a subclass that defines no__init__(inherits the parent's recording wrapper and records under the parent's signature).As per coding guidelines: "For configuration-binding changes, update
tests/unit/test_config.py".🤖 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_api.py` around lines 40 - 53, Add equivalent recording coverage to tests/unit/test_config.py for record_given_arguments: verify **kwargs are flattened into _konfai_given, *args records None, and a subclass without its own __init__ uses the inherited parent signature while recording. Keep the existing test_api.py coverage unchanged.Source: Coding guidelines
56-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for a chain that repeats one stage name.
_chain_treehas three branches for a repeated name: bare, module-qualified, and refusal. None is tested. The mapping form of the second branch is broken (see the comment onkonfai/api.pylines 144-155), and a test would have caught it.🧪 Suggested tests
def test_a_repeated_stage_is_written_module_qualified() -> None: tree = api._chain_tree( [Clip(min_value=0.0), Clip(max_value=1.0)], api._STAGE_MODULES, "chains.CT.CT" ) assert list(tree) == ["Clip", "konfai.data.transform:Clip"] def test_a_thrice_repeated_stage_is_refused() -> None: with pytest.raises(ConfigError, match="three stages"): api._chain_tree( [Clip(min_value=0.0), Clip(max_value=1.0), Clip(min_value=2.0)], api._STAGE_MODULES, "chains.CT.CT", ) def test_a_repeated_mapping_stage_is_refused_by_name() -> None: with pytest.raises(ConfigError, match="Clip"): api._chain_tree( [{"Clip": {"min_value": 0.0}}, {"Clip": {"max_value": 1.0}}], api._STAGE_MODULES, "chains.CT.CT", )🤖 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_api.py` around lines 56 - 58, Add coverage in the test suite for _chain_tree’s repeated-stage branches: verify a twice-repeated Clip is emitted once bare and once module-qualified, verify a thrice-repeated stage raises ConfigError mentioning three stages, and verify repeated mapping-form Clip stages are refused with an error naming Clip.
128-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso pin the environment restore on the failure path.
This test covers the success path.
_one_workflow_at_a_timerestores the environment in afinallyblock, and a designed refusal (as intest_a_designed_refusal_raises_instead_of_exiting) is the path most likely to regress. Assert that theKONFAI_*keys and the lock are both released after the raise.🤖 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_api.py` around lines 128 - 139, Extend test_the_environment_is_left_as_found to exercise a designed refusal that raises, then assert all KONFAI_* environment keys are absent and the workflow lock is released afterward. Reuse the refusal setup and lock symbol from test_a_designed_refusal_raises_instead_of_exiting, while preserving the existing success-path assertions.konfai/trainer.py (1)
1096-1096: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThree build functions widened the config parameter to
Path | str | dictwithout updating their numpydoc blocks.build_transformdocuments the dict form;build_train,build_predictandbuild_evaluatedo not, so the rendered API reference still says the parameter is a path.
konfai/trainer.py#L1096-L1096: update theconfigdescription at lines 1110-1111 toPath | str | dictand state that a dict is the config tree itself.konfai/predictor.py#L2254-L2254: update theprediction_filedescription at lines 2264-2265 the same way.konfai/evaluator.py#L651-L651: update theevaluations_filedescription at lines 659-660 the same way.🤖 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/trainer.py` at line 1096, Update the numpydoc parameter descriptions for build_train’s config, predictor.py’s prediction_file, and evaluator.py’s evaluations_file to document the type as Path | str | dict and state that a dict is the configuration tree itself; update all three listed sites consistently.konfai/data/transform.py (1)
286-299: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDocument the
write_stream_cache_attributeAPI change.All in-repository overrides accept
name. ExternalTransformsubclasses with the previous two-argument signature will raiseTypeErrorwhen callers passnamepositionally. Add a release note for this extension-point change.🤖 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 286 - 299, Add a release note documenting that the Transform.write_stream_cache_attribute extension point now accepts the case name parameter, name. State that external Transform subclasses retaining the previous two-argument signature must update their override to accept name because callers may pass it positionally.
🤖 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 `@apps/impact_reg/impact_reg_konfai/impact_reg.py`:
- Around line 690-703: Before writing the `Uncertainty` dataset in the
`impact_reg_uncertainty` transform, call `_output_path` to remove any stale
output with the same stem in the alternate format. Match the cleanup behavior
used by `_ensemble_mean` and `_derive_moved`, while preserving the existing
format selection and `Write` configuration.
- Around line 496-515: Update _derive_moved so mixed DVF suffixes cannot cause
one case’s Write format to be applied to every case: validate that all cases in
cases share the same suffix/form before _run_transform proceeds, and fail
clearly when they differ. Preserve each case’s existing _output_path cleanup and
use the validated common form for the Write dataset format.
In `@konfai/api.py`:
- Around line 259-271: Add a return annotation of
konfai.transformer.TransformPlan to the public plan_transform function,
importing TransformPlan under TYPE_CHECKING so runtime imports remain
lightweight.
- Around line 144-155: Update the duplicate-name handling in the stage-tree
construction loop to derive the module-qualified name from the resolved stage
entry returned by _stage_entry, rather than from the runtime type of the stage
container. Preserve the existing three-spellings ConfigError behavior and ensure
repeated one-entry mappings qualify using their entry name.
- Around line 317-323: Update the group derivation before the groups_src
construction to expand each target key by splitting it on “;” and include every
component in groups, while retaining metric source groups. Ensure composite
targets such as Seg;Mask produce separate Seg and Mask entries for Evaluator
validation.
In `@konfai/metric/measure.py`:
- Around line 78-83: Update record_given_arguments and the __init_subclass__
path for Metric subclasses so classes inheriting Criterion.__init__, such as
Accuracy, receive a wrapped constructor and populate _konfai_given when
instantiated. Preserve existing behavior for classes with local constructors,
and add a regression test verifying that an inherited-constructor metric can be
serialized in a live workflow.
In `@konfai/utils/runtime.py`:
- Around line 184-204: Update _materialized_config to retain the scratch
directory path and register it for process-exit cleanup, using the appropriate
tempfile cleanup mechanism. Keep the directory available through workflow
execution so resolved defaults can still be written, while ensuring each
generated scratch directory is removed after the process exits.
In `@tests/unit/test_api.py`:
- Around line 211-217: Update test_a_config_tree_must_hold_the_workflow_root to
verify that the temporary directory created by _materialized_config is cleaned
up after materialization, replacing the current path.is_file-only assertion as
needed. Preserve the existing ConfigError validation and successful Transformer
configuration coverage, and align the assertion with the cleanup behavior
implemented by _materialized_config.
---
Nitpick comments:
In `@konfai/data/reduction.py`:
- Around line 165-166: Declare working_multiple = 2.0 alongside voxel_local and
incremental for the Std reduction configuration, so the planner allocates four
regions including the output.
In `@konfai/data/transform.py`:
- Around line 286-299: Add a release note documenting that the
Transform.write_stream_cache_attribute extension point now accepts the case name
parameter, name. State that external Transform subclasses retaining the previous
two-argument signature must update their override to accept name because callers
may pass it positionally.
In `@konfai/trainer.py`:
- Line 1096: Update the numpydoc parameter descriptions for build_train’s
config, predictor.py’s prediction_file, and evaluator.py’s evaluations_file to
document the type as Path | str | dict and state that a dict is the
configuration tree itself; update all three listed sites consistently.
In `@tests/unit/test_api.py`:
- Around line 40-53: Add equivalent recording coverage to
tests/unit/test_config.py for record_given_arguments: verify **kwargs are
flattened into _konfai_given, *args records None, and a subclass without its own
__init__ uses the inherited parent signature while recording. Keep the existing
test_api.py coverage unchanged.
- Around line 56-58: Add coverage in the test suite for _chain_tree’s
repeated-stage branches: verify a twice-repeated Clip is emitted once bare and
once module-qualified, verify a thrice-repeated stage raises ConfigError
mentioning three stages, and verify repeated mapping-form Clip stages are
refused with an error naming Clip.
- Around line 128-139: Extend test_the_environment_is_left_as_found to exercise
a designed refusal that raises, then assert all KONFAI_* environment keys are
absent and the workflow lock is released afterward. Reuse the refusal setup and
lock symbol from test_a_designed_refusal_raises_instead_of_exiting, while
preserving the existing success-path assertions.
🪄 Autofix
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: e735a4bd-4150-4da1-bade-875aed937e78
📒 Files selected for processing (21)
.claude/skills/konfai-experiments/references/tool-reference.mdapps/impact_reg/impact_reg_konfai/cli.pyapps/impact_reg/impact_reg_konfai/impact_reg.pyapps/impact_reg/tests/unit/test_displacement_field_io.pyapps/impact_reg/tests/unit/test_orchestration.pyapps/impact_reg/tests/unit/test_tmp_dir_forwarding.pykonfai-mcp/konfai_mcp/guide.pykonfai/__init__.pykonfai/api.pykonfai/data/augmentation.pykonfai/data/patching.pykonfai/data/reduction.pykonfai/data/transform.pykonfai/evaluator.pykonfai/metric/measure.pykonfai/predictor.pykonfai/trainer.pykonfai/transformer.pykonfai/utils/config.pykonfai/utils/runtime.pytests/unit/test_api.py
b822331 to
5b8c6ea
Compare
5b8c6ea to
4337e9c
Compare
The workflows become Python callables:
konfai.transform/plan_transform/evaluate/predict/trainaccept live stage objects or a config tree as a dict, build the same tree the YAML file would hold, and hand it to the same binder. Refusals raiseKonfAIError; results come back structured; the resolved YAML is still written to the workspace.Supporting pieces in the same line:
3ac59c3feat(data): a reference that follows the case —reference: "{case}"resolves per case5df9455feat(data): Std reduction and Magnitude transform — the streamed-uncertainty vocabularyc6ca8e4feat(api): the workflows as Python callables79ef557feat(impact-reg): every derivation through konfai's own engine — the orchestrator's moved/mean/uncertainty/warp all run as TRANSFORM chains, fully out of corea7b429fdocs(mcp): plan_transform's verdict list gains LOADStacked on
feat/impact-reg-cases-and-fields.