Refactor (gradio part 6) mode wiring decomposition#663
Merged
ChuxiJ merged 5 commits intoace-step:mainfrom Feb 21, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR refactors Gradio event handler registration by decomposing monolithic wiring logic from the main events module into specialized, modularized wiring modules. Large blocks of direct event callback definitions are replaced with calls to new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
acestep/ui/gradio/events/wiring/training_run_wiring.py (4)
45-47: Use f-string conversion flag instead ofstr(exc).Static analysis (RUF010) suggests using the
!sconversion flag for cleaner f-string formatting.♻️ Proposed fix
- except Exception as exc: # pragma: no cover - defensive UI wrapper - logger.exception("Training wrapper error") - yield f"❌ Error: {str(exc)}", str(exc), None, state + except Exception as exc: # pragma: no cover - defensive UI wrapper + logger.exception("Training wrapper error") + yield f"❌ Error: {exc!s}", f"{exc!s}", None, state🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/ui/gradio/events/wiring/training_run_wiring.py` around lines 45 - 47, Replace explicit str(exc) conversions with f-string conversion flags to follow RUF010: in the except block inside training_run_wiring (the exception handler that logs "Training wrapper error" and yields the error tuple), use the !s conversion on the exception in the f-string and the second yielded value (e.g., yield f"❌ Error: {exc!s}", exc!s, None, state) so the exception is converted via the f-string conversion flag instead of calling str().
21-36: Use descriptive parameter names for wrapper function signatures.The abbreviated names (
r,a,d,lr,ep,bs,ga,se,sh,sd,od,rc,ts) are cryptic and hinder readability. Since this is a public wrapper that streams UI progress, meaningful names would improve maintainability.♻️ Suggested parameter naming
def training_wrapper( tensor_dir: Any, - r: Any, - a: Any, - d: Any, - lr: Any, - ep: Any, - bs: Any, - ga: Any, - se: Any, - sh: Any, - sd: Any, - od: Any, - rc: Any, - ts: Any, + lora_rank: Any, + lora_alpha: Any, + lora_dropout: Any, + learning_rate: Any, + epochs: Any, + batch_size: Any, + gradient_accumulation: Any, + save_every: Any, + shift: Any, + seed: Any, + output_dir: Any, + resume_checkpoint: Any, + training_state: Any, ) -> Iterator[tuple[Any, Any, Any, dict[str, bool]]]:As per coding guidelines: "Use clear names that describe behavior, not implementation trivia in Python code."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/ui/gradio/events/wiring/training_run_wiring.py` around lines 21 - 36, The training_wrapper function signature uses cryptic short parameter names (tensor_dir, r, a, d, lr, ep, bs, ga, se, sh, sd, od, rc, ts) which reduces readability; rename these parameters to descriptive names (e.g., replace r->run_id or record, a->artifact_path or assets, d->dataset, lr->learning_rate, ep->epochs, bs->batch_size, ga->grad_accumulation, se->seed, sh->shuffle, sd->save_dir, od->output_dir, rc->resume_checkpoint, ts->timestamp) and update all internal references and any callers to use the new names to preserve behavior in training_wrapper (and its return type Iterator[tuple[Any, Any, Any, dict[str, bool]]]).
1-226: Module exceeds the 200 LOC hard cap.At ~226 lines, this module exceeds the established guideline. Consider extracting the LoKr wiring (lines 164-226) into a separate
training_lokr_wiring.pymodule to improve maintainability.Based on learnings: "only raise module-size concerns when a file exceeds 200 lines of code (LOC)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/ui/gradio/events/wiring/training_run_wiring.py` around lines 1 - 226, This module is over the 200-line cap; extract the LoKr-specific wiring into a new training_lokr_wiring.py to keep register_training_run_handlers small: move _build_lokr_training_wrapper and all LoKr-related click registrations (references: _build_lokr_training_wrapper, lokr_training_wrapper, start_lokr_training_btn, stop_lokr_training_btn, refresh_lokr_export_epochs_btn, export_lokr_btn, and the training_section keys prefixed with "lokr_") into that new module, export a function (e.g., register_lokr_training_handlers(context)) that registers only those handlers, and replace the LoKr block in register_training_run_handlers with a single call to the new register_lokr_training_handlers while importing it at top; ensure _normalize_training_state and non-LoKr wiring remain in the original file and update any imports/exports accordingly.
99-101: Use f-string conversion flag here as well.Same RUF010 issue as the standard training wrapper.
♻️ Proposed fix
- except Exception as exc: # pragma: no cover - defensive UI wrapper - logger.exception("LoKr training wrapper error") - yield f"❌ Error: {str(exc)}", str(exc), None, state + except Exception as exc: # pragma: no cover - defensive UI wrapper + logger.exception("LoKr training wrapper error") + yield f"❌ Error: {exc!s}", f"{exc!s}", None, state🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/ui/gradio/events/wiring/training_run_wiring.py` around lines 99 - 101, Replace the plain str(exc) usage with the f-string conversion flag !r so exception objects are represented with their repr; specifically update the yield line that currently yields f"❌ Error: {str(exc)}", str(exc), None, state to use f"❌ Error: {exc!r}", exc!r, None, state and ensure the logger.exception("LoKr training wrapper error") line remains (it already logs the traceback) so the human-readable and machine-friendly exception representation comes from using {exc!r} in the yield.acestep/ui/gradio/events/wiring/decomposition_contract_test.py (1)
1-314: Consider splitting test file if it grows further.At ~314 LOC, this test file is approaching complexity limits. If more wiring modules are added, consider splitting into
decomposition_contract_generation_test.pyanddecomposition_contract_training_test.pyto maintain focused test scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/ui/gradio/events/wiring/decomposition_contract_test.py` around lines 1 - 314, Split the large DecompositionContractTests into two focused test files: create decomposition_contract_generation_test.py containing the generation-related tests (keep methods test_setup_event_handlers_uses_generation_wiring_helpers, test_generation_metadata_file_wiring_calls_expected_handlers, test_generation_mode_wiring_uses_mode_ui_outputs_variable, test_generation_run_wiring_calls_expected_results_handlers, test_batch_navigation_wiring_calls_expected_results_handlers, test_results_display_wiring_calls_expected_results_handlers) and create decomposition_contract_training_test.py containing the training-related tests (keep methods test_setup_training_event_handlers_uses_training_run_wiring_helper, test_training_run_wiring_calls_expected_training_handlers, test_training_dataset_builder_wiring_calls_expected_handlers, test_training_dataset_preprocess_wiring_calls_expected_handlers); extract the top-level helpers and path constants (functions like _load_setup_event_handlers_node, _load_setup_training_event_handlers_node, _load_generation_mode_wiring_node, _load_generation_run_wiring_node, _load_generation_batch_navigation_wiring_node, _load_generation_metadata_file_wiring_module, _load_training_run_wiring_module, _load_training_dataset_preprocess_wiring_module, _load_training_dataset_builder_wiring_module, _load_results_display_wiring_module and constants like _EVENTS_INIT_PATH etc.) into a small shared module (e.g., decomposition_contract_helpers.py) and import those helpers from both new test files so each test file is concise and focused.acestep/ui/gradio/events/wiring/results_display_wiring_test.py (1)
50-61: Consider extracting shared AST helpers if pattern grows.The
_load_module_asthelper mirrors the one ingeneration_metadata_file_wiring_test.py. For two occurrences, inline duplication is acceptable; if more test files adopt this pattern, consider a sharedtest_utilsmodule.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/ui/gradio/events/wiring/results_display_wiring_test.py` around lines 50 - 61, The helper _load_module_ast (and related helpers like _subscript_key) is duplicated in another test file; extract these shared AST utilities into a common test utility module (e.g., test_utils or tests/_ast_helpers) and import them from there in results_display_wiring_test.py and generation_metadata_file_wiring_test.py; move the functions _load_module_ast and _subscript_key into the new module, keep the behavior (reading with encoding="utf-8" and returning ast.Module), update the tests to import the helpers, and run tests to ensure no import/name conflicts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@acestep/ui/gradio/events/wiring/docstring_coverage_test.py`:
- Line 47: The current assignment module_name =
str(module_path.relative_to(Path.cwd())) can raise ValueError when tests run
outside the repo root; update the logic around the module_name calculation (the
module_path -> module_name conversion) to be defensive: attempt
module_path.relative_to(Path.cwd()) in a try block and on ValueError fall back
to a stable identifier such as module_path.name (or module_path.as_posix()),
ensuring module_path and module_name are used accordingly so callers still
receive a readable module identifier.
In `@acestep/ui/gradio/events/wiring/results_display_wiring.py`:
- Line 71: Update the stale inline comment next to the setTimeout call that
currently reads "// 300ms interval to avoid browser blocking" — change it to
reflect the actual delay used (index * 1000), e.g., "// 1000ms interval per
index to avoid browser blocking" or remove the misleading duration; locate the
setTimeout invocation referencing "index * 1000" in results_display_wiring.py
and update its comment accordingly.
In `@acestep/ui/gradio/events/wiring/training_dataset_builder_wiring.py`:
- Line 94: The f-string in the lambda (fn=lambda status: f"{status or '\u2705
Auto-label complete.'}\n\u2705 Preview refreshed.") uses a \u escape inside an
f-string which is invalid on Python 3.11; replace the escape by extracting the
checkmark emoji to a constant (e.g., CHECKMARK = "✅") or use the emoji literal,
then rebuild the string using that symbol (e.g., use the CHECKMARK variable in
the lambda expression for both the "Auto-label complete." and "Preview
refreshed." parts) so the f-string contains no \u escape sequences.
---
Nitpick comments:
In `@acestep/ui/gradio/events/wiring/decomposition_contract_test.py`:
- Around line 1-314: Split the large DecompositionContractTests into two focused
test files: create decomposition_contract_generation_test.py containing the
generation-related tests (keep methods
test_setup_event_handlers_uses_generation_wiring_helpers,
test_generation_metadata_file_wiring_calls_expected_handlers,
test_generation_mode_wiring_uses_mode_ui_outputs_variable,
test_generation_run_wiring_calls_expected_results_handlers,
test_batch_navigation_wiring_calls_expected_results_handlers,
test_results_display_wiring_calls_expected_results_handlers) and create
decomposition_contract_training_test.py containing the training-related tests
(keep methods
test_setup_training_event_handlers_uses_training_run_wiring_helper,
test_training_run_wiring_calls_expected_training_handlers,
test_training_dataset_builder_wiring_calls_expected_handlers,
test_training_dataset_preprocess_wiring_calls_expected_handlers); extract the
top-level helpers and path constants (functions like
_load_setup_event_handlers_node, _load_setup_training_event_handlers_node,
_load_generation_mode_wiring_node, _load_generation_run_wiring_node,
_load_generation_batch_navigation_wiring_node,
_load_generation_metadata_file_wiring_module, _load_training_run_wiring_module,
_load_training_dataset_preprocess_wiring_module,
_load_training_dataset_builder_wiring_module,
_load_results_display_wiring_module and constants like _EVENTS_INIT_PATH etc.)
into a small shared module (e.g., decomposition_contract_helpers.py) and import
those helpers from both new test files so each test file is concise and focused.
In `@acestep/ui/gradio/events/wiring/results_display_wiring_test.py`:
- Around line 50-61: The helper _load_module_ast (and related helpers like
_subscript_key) is duplicated in another test file; extract these shared AST
utilities into a common test utility module (e.g., test_utils or
tests/_ast_helpers) and import them from there in results_display_wiring_test.py
and generation_metadata_file_wiring_test.py; move the functions _load_module_ast
and _subscript_key into the new module, keep the behavior (reading with
encoding="utf-8" and returning ast.Module), update the tests to import the
helpers, and run tests to ensure no import/name conflicts.
In `@acestep/ui/gradio/events/wiring/training_run_wiring.py`:
- Around line 45-47: Replace explicit str(exc) conversions with f-string
conversion flags to follow RUF010: in the except block inside
training_run_wiring (the exception handler that logs "Training wrapper error"
and yields the error tuple), use the !s conversion on the exception in the
f-string and the second yielded value (e.g., yield f"❌ Error: {exc!s}", exc!s,
None, state) so the exception is converted via the f-string conversion flag
instead of calling str().
- Around line 21-36: The training_wrapper function signature uses cryptic short
parameter names (tensor_dir, r, a, d, lr, ep, bs, ga, se, sh, sd, od, rc, ts)
which reduces readability; rename these parameters to descriptive names (e.g.,
replace r->run_id or record, a->artifact_path or assets, d->dataset,
lr->learning_rate, ep->epochs, bs->batch_size, ga->grad_accumulation, se->seed,
sh->shuffle, sd->save_dir, od->output_dir, rc->resume_checkpoint, ts->timestamp)
and update all internal references and any callers to use the new names to
preserve behavior in training_wrapper (and its return type Iterator[tuple[Any,
Any, Any, dict[str, bool]]]).
- Around line 1-226: This module is over the 200-line cap; extract the
LoKr-specific wiring into a new training_lokr_wiring.py to keep
register_training_run_handlers small: move _build_lokr_training_wrapper and all
LoKr-related click registrations (references: _build_lokr_training_wrapper,
lokr_training_wrapper, start_lokr_training_btn, stop_lokr_training_btn,
refresh_lokr_export_epochs_btn, export_lokr_btn, and the training_section keys
prefixed with "lokr_") into that new module, export a function (e.g.,
register_lokr_training_handlers(context)) that registers only those handlers,
and replace the LoKr block in register_training_run_handlers with a single call
to the new register_lokr_training_handlers while importing it at top; ensure
_normalize_training_state and non-LoKr wiring remain in the original file and
update any imports/exports accordingly.
- Around line 99-101: Replace the plain str(exc) usage with the f-string
conversion flag !r so exception objects are represented with their repr;
specifically update the yield line that currently yields f"❌ Error: {str(exc)}",
str(exc), None, state to use f"❌ Error: {exc!r}", exc!r, None, state and ensure
the logger.exception("LoKr training wrapper error") line remains (it already
logs the traceback) so the human-readable and machine-friendly exception
representation comes from using {exc!r} in the yield.
acestep/ui/gradio/events/wiring/training_dataset_builder_wiring.py
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
acestep/ui/gradio/events/wiring/training_lokr_wiring.py (1)
61-63: Minor inconsistency in exception formatting.This wrapper uses
{exc!r}(repr) whiletraining_run_wiring.pyline 63 uses{exc!s}(str). Consider aligning both to use the same format for consistency. The!sformat typically produces cleaner user-facing messages.♻️ Optional consistency fix
- yield f"\u274c Error: {exc!r}", f"{exc!r}", None, state + yield f"\u274c Error: {exc!s}", f"{exc!s}", None, state🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/ui/gradio/events/wiring/training_lokr_wiring.py` around lines 61 - 63, The exception message in the LoKr training wrapper (inside the except block that logs via logger.exception and yields the error) uses repr ({exc!r}) for the user-facing yield; change the yield formatting to use str ({exc!s}) to match training_run_wiring.py and produce cleaner user-facing messages—update the yield line that currently yields f"\u274c Error: {exc!r}", f"{exc!r}", None, state to use {exc!s} instead while leaving logger.exception(...) as-is.acestep/ui/gradio/events/wiring/decomposition_contract_helpers.py (1)
29-89: Consider consolidating repetitive loader functions.The loader functions for
FunctionDefnodes (lines 29-89) follow an identical pattern: read file → parse → search for function name → return or raise. This could be consolidated into a generic helper.♻️ Optional refactor to reduce duplication
def _load_function_def(path: Path, function_name: str) -> ast.FunctionDef: """Load a specific FunctionDef from the given module path.""" source = path.read_text(encoding="utf-8") module = ast.parse(source) for node in module.body: if isinstance(node, ast.FunctionDef) and node.name == function_name: return node raise AssertionError(f"{function_name} not found") def load_setup_event_handlers_node() -> ast.FunctionDef: """Return the AST node for ``setup_event_handlers``.""" return _load_function_def(_EVENTS_INIT_PATH, "setup_event_handlers")This would reduce the 5 similar loader functions to concise one-liners.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/ui/gradio/events/wiring/decomposition_contract_helpers.py` around lines 29 - 89, Several loader functions (load_setup_event_handlers_node, load_setup_training_event_handlers_node, load_generation_mode_wiring_node, load_generation_run_wiring_node, load_generation_batch_navigation_wiring_node) duplicate the same read/parse/search pattern; add a single helper _load_function_def(path: Path, function_name: str) that reads the file, ast.parse()s it, finds and returns the ast.FunctionDef or raises AssertionError(f"{function_name} not found"), then replace each of the above function bodies with a one-line return calling _load_function_def with the appropriate _EVENTS_INIT_PATH / _MODE_WIRING_PATH / _RUN_WIRING_PATH / _BATCH_NAV_WIRING_PATH and the target function name; keep load_generation_metadata_file_wiring_module as-is since it returns ast.Module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@acestep/ui/gradio/events/wiring/decomposition_contract_helpers.py`:
- Around line 29-89: Several loader functions (load_setup_event_handlers_node,
load_setup_training_event_handlers_node, load_generation_mode_wiring_node,
load_generation_run_wiring_node, load_generation_batch_navigation_wiring_node)
duplicate the same read/parse/search pattern; add a single helper
_load_function_def(path: Path, function_name: str) that reads the file,
ast.parse()s it, finds and returns the ast.FunctionDef or raises
AssertionError(f"{function_name} not found"), then replace each of the above
function bodies with a one-line return calling _load_function_def with the
appropriate _EVENTS_INIT_PATH / _MODE_WIRING_PATH / _RUN_WIRING_PATH /
_BATCH_NAV_WIRING_PATH and the target function name; keep
load_generation_metadata_file_wiring_module as-is since it returns ast.Module.
In `@acestep/ui/gradio/events/wiring/training_lokr_wiring.py`:
- Around line 61-63: The exception message in the LoKr training wrapper (inside
the except block that logs via logger.exception and yields the error) uses repr
({exc!r}) for the user-facing yield; change the yield formatting to use str
({exc!s}) to match training_run_wiring.py and produce cleaner user-facing
messages—update the yield line that currently yields f"\u274c Error: {exc!r}",
f"{exc!r}", None, state to use {exc!s} instead while leaving
logger.exception(...) as-is.
ChuxiJ
approved these changes
Feb 21, 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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Continue decomposition of
acestep/ui/gradio/events/__init__.pyinto focused wiring modules while preserving behaviour and IO ordering.Changes
training_run_wiring.pytraining_dataset_builder_wiring.pytraining_dataset_preprocess_wiring.pyresults_display_wiring.pygeneration_metadata_file_wiring.pyacestep/ui/gradio/events/__init__.pyto thin orchestration (121 LOC).decomposition_contract_test.pygeneration_metadata_file_wiring_test.pyresults_display_wiring_test.pydocstring_coverage_test.pycontext_test.pyremains green.Behaviour/Interface Impact
Validation
python acestep/ui/gradio/events/wiring/decomposition_contract_test.pypython acestep/ui/gradio/events/wiring/context_test.pypython acestep/ui/gradio/events/wiring/generation_metadata_file_wiring_test.pypython acestep/ui/gradio/events/wiring/results_display_wiring_test.pypython acestep/ui/gradio/events/wiring/docstring_coverage_test.pypython -m py_compile ...on changed modulesCode reviews by GLM 5 raised no issues.
Risk
Summary by CodeRabbit
Refactor
Tests