feat(gradio): add external LM config UI vertical slice#949
Open
1larity wants to merge 10 commits intoace-step:mainfrom
Open
feat(gradio): add external LM config UI vertical slice#9491larity wants to merge 10 commits intoace-step:mainfrom
1larity wants to merge 10 commits intoace-step:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds external-LLM caption-enhancement: credential resolution (env or encrypted store), HTTP client and external format-plan requester with retry/merge logic, LLMHandler routing to external plans, Gradio UI controls/persistence/wiring for external providers, and unit tests covering routing, HTTP validation, credential resolution, and UI wiring. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Gradio UI
participant Handler as Format Handler
participant LLM as LLMHandler
participant Creds as Credential Resolver
participant Client as External HTTP Client
participant Provider as External Provider
UI->>Handler: format_sample_from_input(caption, lyrics)
Handler->>LLM: has_available_text_llm()
alt external backend available
LLM-->>Handler: True
Handler->>LLM: _format_sample_from_external_llm(caption, lyrics)
LLM->>Creds: resolve_external_api_key(provider)
Creds-->>LLM: api_key
LLM->>Client: request_external_format_plan(provider, model, base_url, api_key, caption, lyrics)
Client->>Provider: POST JSON (format intent)
Provider-->>Client: JSON plan response
Client->>Client: parse ExternalAIPlan
alt caption needs retry
Client->>Provider: POST retry with rewrite instruction
Provider-->>Client: improved response
Client->>Client: merge retry plan
end
Client-->>LLM: ExternalAIPlan
LLM-->>Handler: metadata + status
else no available LLM
LLM-->>Handler: False
Handler-->>UI: error status
end
Handler-->>UI: (metadata, status)
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1larity
force-pushed
the
pr808/external-lm-ui-slice
branch
from
5a1f8c1 to
f969afe
Compare
![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: 7
🧹 Nitpick comments (7)
acestep/text_tasks/external_lm_credentials.py (1)
28-34: Make secret-path return contract consistent (Pathvs optional).
resolve_external_secret_store_path()is annotated asPath, but_load_external_api_key_from_secret_store()handlesNone. Please align one way (prefer typed optional ifresolve_existing_default_path(...)can return nullish) to avoid misleading callers.Also applies to: 63-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/text_tasks/external_lm_credentials.py` around lines 28 - 34, The function resolve_external_secret_store_path currently declares a Path return but can yield None via EncryptedSecretStore.resolve_existing_default_path, so change its signature to return Optional[Path> (or otherwise ensure it never returns None) and update callers accordingly; specifically, update resolve_external_secret_store_path's type annotation to Optional[Path] and adjust any uses such as in _load_external_api_key_from_secret_store to match the optional contract (or alternatively make resolve_existing_default_path always return a Path and keep the non-optional signature) so the types are consistent across resolve_external_secret_store_path and _load_external_api_key_from_secret_store.acestep/ui/gradio/events/generation/service_tier_updates.py (1)
16-17: Add explicit type hints and full public-function docstring foron_tier_change.Please annotate inputs/return shape and expand the docstring with Args/Returns to keep this new callback self-describing and easier to validate.
As per coding guidelines: "Type hints: Add type annotations for new/modified functions when practical." and "Docstrings are mandatory for all modules, classes, and public functions. Use concise format with Args, Returns, and exception documentation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/ui/gradio/events/generation/service_tier_updates.py` around lines 16 - 17, Update the on_tier_change function signature to include explicit type hints (e.g., def on_tier_change(selected_tier: Any, llm_handler: Optional[Any] = None) -> None) and import Any and Optional from typing; then expand the existing docstring into a full public-function docstring with a short description plus Args (selected_tier: type and meaning, llm_handler: type and optional behavior), Returns (None) and any raised exceptions, ensuring the docstring uses the project's concise Args/Returns format so the callback is self-describing and type-checkable.acestep/llm_inference_external_format_routing_test.py (1)
21-22: Stub signature doesn't match production keyword-only parameters.The production
_format_sample_from_external_llmuses keyword-only parameters (*, caption, lyrics, user_metadata=None), but this stub uses positional parameters. While the test passes due to how Python handles the binding, the stub doesn't accurately reflect the production contract.♻️ Proposed fix to match production signature
- def fake_external(self, caption, lyrics, user_metadata=None): - return ({"caption": f"external::{caption}", "lyrics": lyrics}, "external-ok") + def fake_external(self, *, caption, lyrics, user_metadata=None): + return ({"caption": f"external::{caption}", "lyrics": lyrics}, "external-ok")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/llm_inference_external_format_routing_test.py` around lines 21 - 22, The test stub fake_external does not match the production function signature _format_sample_from_external_llm which declares keyword-only params; update fake_external to use the same keyword-only signature (i.e., accept *, caption, lyrics, user_metadata=None) and ensure any test invocations supply caption/lyrics as keywords if needed so the stub faithfully mirrors the production contract.acestep/ui/gradio/interfaces/generation_advanced_primary_controls.py (1)
60-83: Consider simplifying the wrapper by returning the implementation result directly.The wrapper explicitly re-maps all 11 keys from the implementation, which creates maintenance overhead if keys change. If the contract requires exactly these keys, consider adding a comment explaining why the explicit mapping is intentional (e.g., to enforce the public API surface).
Alternatively, if the implementation already returns the correct keys:
♻️ Simpler delegation if API surface matches
def build_external_lm_controls( *, service_pre_initialized: bool, params: dict[str, Any], ) -> dict[str, Any]: """Create the external-LM configuration accordion shown above LM controls.""" - components = _build_external_lm_controls_impl( + return _build_external_lm_controls_impl( service_pre_initialized=service_pre_initialized, params=params, ) - return { - "external_llm_accordion": components["external_llm_accordion"], - "external_llm_provider": components["external_llm_provider"], - "external_llm_base_url_preset": components["external_llm_base_url_preset"], - "external_llm_model": components["external_llm_model"], - "external_llm_fetch_models_btn": components["external_llm_fetch_models_btn"], - "external_llm_base_url": components["external_llm_base_url"], - "external_llm_api_key": components["external_llm_api_key"], - "external_llm_status": components["external_llm_status"], - "external_llm_save_btn": components["external_llm_save_btn"], - "external_llm_test_btn": components["external_llm_test_btn"], - "external_llm_doctor_btn": components["external_llm_doctor_btn"], - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/ui/gradio/interfaces/generation_advanced_primary_controls.py` around lines 60 - 83, The wrapper build_external_lm_controls currently re-maps all keys from _build_external_lm_controls_impl; simplify by returning the implementation result directly (i.e., return components) to avoid maintenance overhead, or if you must enforce an explicit public API, add a short comment above the mapping explaining that the explicit key list is intentional to lock the public contract and keep the current explicit return. Reference build_external_lm_controls and _build_external_lm_controls_impl when making this change.acestep/ui/gradio/interfaces/generation_external_lm_controls.py (1)
21-41: Missing blank lines between top-level definitions.Per PEP 8, there should be two blank lines between top-level function definitions and after imports. Lines 20-21, 127-128, and 176-177 are missing these separators.
However, since no static analysis hints flag this and the code is functional, this is a minor style nit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/ui/gradio/interfaces/generation_external_lm_controls.py` around lines 21 - 41, The file is missing the required two blank lines between top-level definitions per PEP8; add a blank line separation (two newlines) before and after the top-level function build_external_lm_controls and similarly insert the two-line separators between other top-level definitions referenced in the review so that there are two blank lines after imports and between each top-level def to satisfy PEP8.acestep/ui/gradio/events/wiring/generation_service_wiring_sections.py (1)
4-11: Missing blank lines between top-level function definitions.Per PEP 8, there should be two blank lines between top-level function definitions. This affects readability but doesn't impact functionality.
Also applies to: 12-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/ui/gradio/events/wiring/generation_service_wiring_sections.py` around lines 4 - 11, The top-level function definitions (e.g., register_dataset_handlers and the subsequent top-level functions starting around lines 12-33) lack the required two blank lines between them per PEP 8; update the file so there are exactly two blank lines separating each top-level function definition (add one additional blank line where only one exists) to restore proper spacing and improve readability.acestep/text_tasks/external_lm_format_client_test.py (1)
45-67: Patch targets may cause test fragility.The test patches
EncryptedSecretStoreon theexternal_lm_credentialsmodule but then also patchesresolve_existing_default_pathon the class itself. This dual-patching approach works but could become fragile if the import structure changes.Consider patching at the point of use (
external_lm_format_client) instead:`@patch`("acestep.text_tasks.external_lm_format_client.resolve_runtime_passphrase") `@patch`("acestep.text_tasks.external_lm_format_client.EncryptedSecretStore")The static analysis hint about "hardcoded password" on line 67 is a false positive—this is a mock value for testing, not a production credential.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/text_tasks/external_lm_format_client_test.py` around lines 45 - 67, The test test_resolve_external_api_key_uses_secure_store_fallback should patch the symbols where resolve_external_api_key actually looks them up, so change the decorators to patch "acestep.text_tasks.external_lm_format_client.resolve_runtime_passphrase" and "acestep.text_tasks.external_lm_format_client.EncryptedSecretStore" (and similarly patch EncryptedSecretStore.resolve_existing_default_path on that same module) instead of patching the ones in external_lm_credentials; keep the rest of the test logic unchanged and, if your linter flags the mock passphrase/secret as a hardcoded credential, add a short test-only comment or disable rule locally to mark it as a mock.
🤖 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/text_tasks/external_lm_format_client.py`:
- Around line 75-99: When a retry is triggered by caption_needs_retry, make the
retry best-effort: call _request_plan_once inside a try/except so timeouts/parse
errors are caught and logged instead of bubbling out, and do not replace plan
wholesale—merge only non-empty fields from retry_plan (e.g., caption, metadata,
lyrics) into the existing plan object so the original plan remains the fallback;
reference the caption_needs_retry check, the retry_intent construction,
_request_plan_once call, and the plan/retry_plan variables when making these
changes.
In `@acestep/text_tasks/external_lm_http_client.py`:
- Around line 28-36: The code constructs a Request and calls urlopen_impl with a
user-provided base_url without validating the URL scheme, which allows file://
or other dangerous schemes; before creating the urllib_request.Request (and
before calling urlopen_impl/Request), parse base_url (e.g., via
urllib.parse.urlparse) and enforce an allowlist of schemes (only "http" and
"https"), and ensure a valid network location (netloc) is present; if validation
fails, raise a clear exception or return an error instead of performing the
request so urlopen_impl(Request, ...) is never invoked with unsafe schemes.
In `@acestep/ui/gradio/events/generation/external_lm_setup_actions.py`:
- Around line 26-39: The try block around model discovery also contains
save_cache, so a cache write failure can mask a successful discover_models call;
refactor by keeping get_profile and discover_models inside the existing try that
catches ValueError and ExternalModelDiscoveryError, then perform
save_cache(profile=profile.provider_id ???) in a separate try/except that only
handles cache-related errors (e.g., OSError, IOError) and logs or surfaces a
non-fatal warning via status_formatter/t(...) without returning an error; ensure
you do not use a bare except and that the successful discovery path still
returns the models even if save_cache fails, referencing get_profile,
discover_models, save_cache, status_formatter, t, and
ExternalModelDiscoveryError to locate the spots to change.
In `@acestep/ui/gradio/events/generation/service_init_helpers.py`:
- Around line 56-81: The code flips llm_handler to external but doesn't unload
any resident local model, leaving weights in memory; before setting
llm_handler.llm_backend = "external" and llm_handler.llm_initialized = False,
call a cleanup/unload routine on llm_handler (e.g., if
callable(getattr(llm_handler, "unload_local_llm", None)):
llm_handler.unload_local_llm()) or, if no dedicated method exists, explicitly
release known resources on llm_handler (close processes, free model
objects/handles, remove GPU contexts) using attribute names present on the
handler; ensure this unload is executed prior to changing backend/init flags so
no local weights remain resident.
In `@acestep/ui/gradio/events/generation/service_init.py`:
- Around line 41-55: update_llm_backend_ui currently forces the init_llm
checkbox to True for external backends but never restores it when switching
back, leaving init_llm "sticky-checked"; change update_llm_backend_ui to accept
or obtain the prior/init default value for the init_llm checkbox (e.g., add a
parameter prev_init_llm: bool or compute tier default based on backend) and when
not external set checkbox_update = gr.update(info=t(info_key),
value=prev_init_llm) (or value=<tier_default>) instead of leaving value
unset/always True so the checkbox is restored to the previous or tier-default
state; update references to checkbox_update and callers of update_llm_backend_ui
accordingly.
In `@acestep/ui/gradio/events/generation/service_tier_updates.py`:
- Around line 53-54: The new user-facing literals must be localized: replace the
plain string assigned to recommended_suffix and the other new status/info
literals (the LM availability note and the batch/duration info added in the same
block) with calls to the localization helper (e.g., t("...")) so they use the
app's translation pipeline; update occurrences where these variables or literals
are displayed to use the translated values (refer to recommended_suffix and the
nearby LM availability and batch/duration message variables in
service_tier_updates.py).
In `@acestep/ui/gradio/interfaces/generation_service_config_rows.py`:
- Around line 168-184: Normalize the backend once into backend_value and reuse
that normalized value for both the local_lm_column visibility and the
backend_dropdown default value to prevent UI state drift; specifically, compute
backend_value by trimming and lowercasing params.get("backend",
recommended_backend) (or falling back to recommended_backend) and then use this
same backend_value for the visibility check on local_lm_column and as the value
passed to the backend_dropdown Dropdown (referencing backend_value,
local_lm_column, backend_dropdown, params, and recommended_backend).
---
Nitpick comments:
In `@acestep/llm_inference_external_format_routing_test.py`:
- Around line 21-22: The test stub fake_external does not match the production
function signature _format_sample_from_external_llm which declares keyword-only
params; update fake_external to use the same keyword-only signature (i.e.,
accept *, caption, lyrics, user_metadata=None) and ensure any test invocations
supply caption/lyrics as keywords if needed so the stub faithfully mirrors the
production contract.
In `@acestep/text_tasks/external_lm_credentials.py`:
- Around line 28-34: The function resolve_external_secret_store_path currently
declares a Path return but can yield None via
EncryptedSecretStore.resolve_existing_default_path, so change its signature to
return Optional[Path> (or otherwise ensure it never returns None) and update
callers accordingly; specifically, update resolve_external_secret_store_path's
type annotation to Optional[Path] and adjust any uses such as in
_load_external_api_key_from_secret_store to match the optional contract (or
alternatively make resolve_existing_default_path always return a Path and keep
the non-optional signature) so the types are consistent across
resolve_external_secret_store_path and _load_external_api_key_from_secret_store.
In `@acestep/text_tasks/external_lm_format_client_test.py`:
- Around line 45-67: The test
test_resolve_external_api_key_uses_secure_store_fallback should patch the
symbols where resolve_external_api_key actually looks them up, so change the
decorators to patch
"acestep.text_tasks.external_lm_format_client.resolve_runtime_passphrase" and
"acestep.text_tasks.external_lm_format_client.EncryptedSecretStore" (and
similarly patch EncryptedSecretStore.resolve_existing_default_path on that same
module) instead of patching the ones in external_lm_credentials; keep the rest
of the test logic unchanged and, if your linter flags the mock passphrase/secret
as a hardcoded credential, add a short test-only comment or disable rule locally
to mark it as a mock.
In `@acestep/ui/gradio/events/generation/service_tier_updates.py`:
- Around line 16-17: Update the on_tier_change function signature to include
explicit type hints (e.g., def on_tier_change(selected_tier: Any, llm_handler:
Optional[Any] = None) -> None) and import Any and Optional from typing; then
expand the existing docstring into a full public-function docstring with a short
description plus Args (selected_tier: type and meaning, llm_handler: type and
optional behavior), Returns (None) and any raised exceptions, ensuring the
docstring uses the project's concise Args/Returns format so the callback is
self-describing and type-checkable.
In `@acestep/ui/gradio/events/wiring/generation_service_wiring_sections.py`:
- Around line 4-11: The top-level function definitions (e.g.,
register_dataset_handlers and the subsequent top-level functions starting around
lines 12-33) lack the required two blank lines between them per PEP 8; update
the file so there are exactly two blank lines separating each top-level function
definition (add one additional blank line where only one exists) to restore
proper spacing and improve readability.
In `@acestep/ui/gradio/interfaces/generation_advanced_primary_controls.py`:
- Around line 60-83: The wrapper build_external_lm_controls currently re-maps
all keys from _build_external_lm_controls_impl; simplify by returning the
implementation result directly (i.e., return components) to avoid maintenance
overhead, or if you must enforce an explicit public API, add a short comment
above the mapping explaining that the explicit key list is intentional to lock
the public contract and keep the current explicit return. Reference
build_external_lm_controls and _build_external_lm_controls_impl when making this
change.
In `@acestep/ui/gradio/interfaces/generation_external_lm_controls.py`:
- Around line 21-41: The file is missing the required two blank lines between
top-level definitions per PEP8; add a blank line separation (two newlines)
before and after the top-level function build_external_lm_controls and similarly
insert the two-line separators between other top-level definitions referenced in
the review so that there are two blank lines after imports and between each
top-level def to satisfy PEP8.
🪄 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: 820a4a6e-e6f7-432d-a4e6-783fbb3ca1d9
📒 Files selected for processing (42)
acestep/inference.pyacestep/llm_inference.pyacestep/llm_inference_external_format_routing_test.pyacestep/text_tasks/external_ai_request_helpers.pyacestep/text_tasks/external_lm_captioning.pyacestep/text_tasks/external_lm_captioning_test.pyacestep/text_tasks/external_lm_credentials.pyacestep/text_tasks/external_lm_format_client.pyacestep/text_tasks/external_lm_format_client_test.pyacestep/text_tasks/external_lm_http_client.pyacestep/ui/gradio/events/generation/__init__.pyacestep/ui/gradio/events/generation/external_lm_setup.pyacestep/ui/gradio/events/generation/external_lm_setup_actions.pyacestep/ui/gradio/events/generation/external_lm_setup_support.pyacestep/ui/gradio/events/generation/external_lm_setup_test.pyacestep/ui/gradio/events/generation/llm_format_actions.pyacestep/ui/gradio/events/generation/service_init.pyacestep/ui/gradio/events/generation/service_init_helpers.pyacestep/ui/gradio/events/generation/service_init_test.pyacestep/ui/gradio/events/generation/service_tier_updates.pyacestep/ui/gradio/events/generation_handlers.pyacestep/ui/gradio/events/generation_handlers_test.pyacestep/ui/gradio/events/wiring/generation_service_wiring.pyacestep/ui/gradio/events/wiring/generation_service_wiring_sections.pyacestep/ui/gradio/events/wiring/generation_service_wiring_support.pyacestep/ui/gradio/events/wiring/generation_service_wiring_test.pyacestep/ui/gradio/i18n/en.jsonacestep/ui/gradio/i18n/he.jsonacestep/ui/gradio/i18n/ja.jsonacestep/ui/gradio/i18n/zh.jsonacestep/ui/gradio/interfaces/__init__.pyacestep/ui/gradio/interfaces/generation_advanced_dit_controls.pyacestep/ui/gradio/interfaces/generation_advanced_output_controls.pyacestep/ui/gradio/interfaces/generation_advanced_primary_controls.pyacestep/ui/gradio/interfaces/generation_advanced_settings.pyacestep/ui/gradio/interfaces/generation_decomposition_contract_test.pyacestep/ui/gradio/interfaces/generation_external_lm_config_test.pyacestep/ui/gradio/interfaces/generation_external_lm_controls.pyacestep/ui/gradio/interfaces/generation_service_config_rows.pyacestep/ui/gradio/interfaces/generation_service_config_toggles.pyacestep/ui/gradio/interfaces/generation_tab_optional_controls.pyacestep/ui/gradio/interfaces/generation_tab_source_controls.py
💤 Files with no reviewable changes (3)
- acestep/ui/gradio/interfaces/generation_tab_optional_controls.py
- acestep/ui/gradio/interfaces/generation_tab_source_controls.py
- acestep/inference.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (6)
acestep/text_tasks/external_lm_format_client.py (1)
176-177: Minor: Missing blank line between function definitions.PEP 8 recommends two blank lines between top-level function definitions.
✏️ Formatting fix
content = extract_protocol_message_content(raw_response=raw_response, protocol=protocol) return parse_plan_from_content(content, task_focus="format") + + def _post_external_request(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/text_tasks/external_lm_format_client.py` around lines 176 - 177, There is no blank line between the end of parse_plan_from_content usage and the next top-level function _post_external_request; add one additional blank line so there are two blank lines separating the top-level definitions (ensure the line after the return parse_plan_from_content(content, task_focus="format") is blank before def _post_external_request).acestep/ui/gradio/events/generation/external_lm_setup_actions.py (1)
185-204: Consider extracting_has_secret_store_valueto reduce module size.The module is at 204 LOC, slightly over the 200 LOC hard cap. The
_has_secret_store_valuehelper could be moved toexternal_lm_setup_support.pyto bring this under the cap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/ui/gradio/events/generation/external_lm_setup_actions.py` around lines 185 - 204, The module exceeds the 200 LOC cap because the helper _has_secret_store_value is defined inline; extract this function into external_lm_setup_support.py and import it where needed. Move the full function (including its dependencies: EncryptedSecretStore, SecretStoreError, resolve_runtime_passphrase) into external_lm_setup_support.py, update the original file to replace the local definition with "from .external_lm_setup_support import _has_secret_store_value", and ensure any relative imports and unit tests still resolve; run linter/tests to verify no missing imports or name collisions.acestep/text_tasks/external_lm_format_client_test.py (1)
1-215: Module exceeds 200 LOC hard cap (215 LOC).Per coding guidelines, modules should target ≤150 LOC with a hard cap of 200. Consider splitting credential-resolution tests (
test_resolve_external_api_key_*) into a separate test module from the plan-request tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/text_tasks/external_lm_format_client_test.py` around lines 1 - 215, The file exceeds the 200-LOC cap because credential-resolution tests are bundled with plan-request tests; split the credential tests into a new test module by moving test_resolve_external_api_key_uses_provider_env_fallback and test_resolve_external_api_key_uses_secure_store_fallback (and their supporting imports/mocks: resolve_external_api_key, ExternalAIClientError, and the patched symbols "acestep.text_tasks.external_lm_credentials.resolve_runtime_passphrase" / "EncryptedSecretStore") into a separate test file (e.g., external_lm_credentials_test) so the original external_lm_format_client_test.py falls under the LOC cap; ensure you keep required imports and the environment patching, update any relative imports if needed, and run test discovery to confirm both modules execute.acestep/ui/gradio/events/generation/external_lm_setup_test.py (1)
1-253: Module significantly exceeds 200 LOC hard cap (253 LOC).Per coding guidelines, this test module should be split by responsibility. Consider separating into:
external_lm_setup_hydration_test.py(hydration tests)external_lm_setup_fetch_test.py(fetch/discovery tests)external_lm_setup_persistence_test.py(save/doctor tests)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/ui/gradio/events/generation/external_lm_setup_test.py` around lines 1 - 253, The test module external_lm_setup_test.py is over the 200 LOC cap; split it into three focused test modules: move hydration-related tests (methods: test_hydrate_external_lm_setup_fields_returns_saved_values_and_preset and any mocks calling load_external_lm_runtime_settings/load_cached_external_models) into external_lm_setup_hydration_test.py; move discovery/fetch tests (methods: test_fetch_external_lm_models_updates_dropdown_and_status, test_fetch_external_lm_models_keeps_success_when_cache_write_fails, test_discover_provider_models_uses_resolved_api_key_fallback and any references to fetch_external_lm_models, discover_provider_models, external_lm_setup_support.discover_external_models) into external_lm_setup_fetch_test.py; and move persistence/doctor tests (methods: test_save_external_lm_settings_reports_runtime_only_when_api_key_blank, test_save_external_lm_settings_persists_api_key_when_secret_store_available, test_runtime_doctor_reports_secret_store_presence, test_runtime_doctor_reports_keyring_secret_without_file and any references to save_external_lm_runtime_settings, EncryptedSecretStore, run_external_lm_runtime_doctor, _resolve_secret_store_path, _has_secret_store_value) into external_lm_setup_persistence_test.py; ensure each new file imports only the mocks and symbols it needs (external_lm_setup, external_lm_setup_support, get_external_provider_profile) and retains the original unittest.TestCase structure and patch decorators so tests run unchanged.acestep/ui/gradio/events/generation/service_init_helpers.py (1)
37-43: Fragile path derivation — depends on exact nesting depth.Six nested
dirnamecalls will break if the module is relocated. Consider a more robust approach (e.g., walking upward to find a marker likepyproject.tomlor.git), or document the expected path structure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/ui/gradio/events/generation/service_init_helpers.py` around lines 37 - 43, The resolve_project_root function currently uses six nested os.path.dirname calls which is brittle; modify resolve_project_root to walk upward from current_file checking each parent directory for repository/project markers (e.g., "pyproject.toml", "setup.py", or ".git") and return the first parent that contains any marker; if no marker is found, fall back to the original dirname-based heuristic or raise a clear error. Update references in any callers to handle potential exceptions or None, and keep the function signature resolve_project_root(current_file: str) -> str unchanged.acestep/ui/gradio/events/generation/service_init.py (1)
110-114: Consider catching invalid provider early for a friendlier error.If
external_llm_provideris empty or invalid,get_external_provider_profileraisesValueErrorand the entire init fails with a traceback. A try/except here could return a user-friendly status message instead.Suggested improvement
normalized_backend = (backend or "").strip().lower() external_profile = None if normalized_backend == "external": - external_profile = get_external_provider_profile(external_llm_provider) + try: + external_profile = get_external_provider_profile(external_llm_provider) + except ValueError: + return build_post_init_result( + dit_handler=dit_handler, + llm_handler=llm_handler, + config_path=config_path, + current_mode=current_mode, + current_batch_size=current_batch_size, + status=f"❌ Invalid external LM provider: {external_llm_provider!r}", + enable=False, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/ui/gradio/events/generation/service_init.py` around lines 110 - 114, The code calls get_external_provider_profile(external_llm_provider) without handling ValueError which leads to a raw traceback; wrap that call (in the block using normalized_backend and external_profile) in a try/except catching ValueError, and in the except set external_profile = None and return or populate a clear user-friendly status/message (e.g., "invalid external provider: <name>") instead of letting the exception bubble; specifically change the logic around normalized_backend, external_profile and the get_external_provider_profile call to handle invalid/empty external_llm_provider gracefully.
🤖 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/text_tasks/external_lm_format_client_test.py`:
- Around line 1-215: The file exceeds the 200-LOC cap because
credential-resolution tests are bundled with plan-request tests; split the
credential tests into a new test module by moving
test_resolve_external_api_key_uses_provider_env_fallback and
test_resolve_external_api_key_uses_secure_store_fallback (and their supporting
imports/mocks: resolve_external_api_key, ExternalAIClientError, and the patched
symbols "acestep.text_tasks.external_lm_credentials.resolve_runtime_passphrase"
/ "EncryptedSecretStore") into a separate test file (e.g.,
external_lm_credentials_test) so the original external_lm_format_client_test.py
falls under the LOC cap; ensure you keep required imports and the environment
patching, update any relative imports if needed, and run test discovery to
confirm both modules execute.
In `@acestep/text_tasks/external_lm_format_client.py`:
- Around line 176-177: There is no blank line between the end of
parse_plan_from_content usage and the next top-level function
_post_external_request; add one additional blank line so there are two blank
lines separating the top-level definitions (ensure the line after the return
parse_plan_from_content(content, task_focus="format") is blank before def
_post_external_request).
In `@acestep/ui/gradio/events/generation/external_lm_setup_actions.py`:
- Around line 185-204: The module exceeds the 200 LOC cap because the helper
_has_secret_store_value is defined inline; extract this function into
external_lm_setup_support.py and import it where needed. Move the full function
(including its dependencies: EncryptedSecretStore, SecretStoreError,
resolve_runtime_passphrase) into external_lm_setup_support.py, update the
original file to replace the local definition with "from
.external_lm_setup_support import _has_secret_store_value", and ensure any
relative imports and unit tests still resolve; run linter/tests to verify no
missing imports or name collisions.
In `@acestep/ui/gradio/events/generation/external_lm_setup_test.py`:
- Around line 1-253: The test module external_lm_setup_test.py is over the 200
LOC cap; split it into three focused test modules: move hydration-related tests
(methods: test_hydrate_external_lm_setup_fields_returns_saved_values_and_preset
and any mocks calling
load_external_lm_runtime_settings/load_cached_external_models) into
external_lm_setup_hydration_test.py; move discovery/fetch tests (methods:
test_fetch_external_lm_models_updates_dropdown_and_status,
test_fetch_external_lm_models_keeps_success_when_cache_write_fails,
test_discover_provider_models_uses_resolved_api_key_fallback and any references
to fetch_external_lm_models, discover_provider_models,
external_lm_setup_support.discover_external_models) into
external_lm_setup_fetch_test.py; and move persistence/doctor tests (methods:
test_save_external_lm_settings_reports_runtime_only_when_api_key_blank,
test_save_external_lm_settings_persists_api_key_when_secret_store_available,
test_runtime_doctor_reports_secret_store_presence,
test_runtime_doctor_reports_keyring_secret_without_file and any references to
save_external_lm_runtime_settings, EncryptedSecretStore,
run_external_lm_runtime_doctor, _resolve_secret_store_path,
_has_secret_store_value) into external_lm_setup_persistence_test.py; ensure each
new file imports only the mocks and symbols it needs (external_lm_setup,
external_lm_setup_support, get_external_provider_profile) and retains the
original unittest.TestCase structure and patch decorators so tests run
unchanged.
In `@acestep/ui/gradio/events/generation/service_init_helpers.py`:
- Around line 37-43: The resolve_project_root function currently uses six nested
os.path.dirname calls which is brittle; modify resolve_project_root to walk
upward from current_file checking each parent directory for repository/project
markers (e.g., "pyproject.toml", "setup.py", or ".git") and return the first
parent that contains any marker; if no marker is found, fall back to the
original dirname-based heuristic or raise a clear error. Update references in
any callers to handle potential exceptions or None, and keep the function
signature resolve_project_root(current_file: str) -> str unchanged.
In `@acestep/ui/gradio/events/generation/service_init.py`:
- Around line 110-114: The code calls
get_external_provider_profile(external_llm_provider) without handling ValueError
which leads to a raw traceback; wrap that call (in the block using
normalized_backend and external_profile) in a try/except catching ValueError,
and in the except set external_profile = None and return or populate a clear
user-friendly status/message (e.g., "invalid external provider: <name>") instead
of letting the exception bubble; specifically change the logic around
normalized_backend, external_profile and the get_external_provider_profile call
to handle invalid/empty external_llm_provider gracefully.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7a8bb8ad-1ef8-48bf-b5a2-c3ddbaa7de7b
📒 Files selected for processing (15)
acestep/text_tasks/external_lm_format_client.pyacestep/text_tasks/external_lm_format_client_test.pyacestep/text_tasks/external_lm_http_client.pyacestep/text_tasks/external_lm_http_client_test.pyacestep/ui/gradio/events/generation/external_lm_setup_actions.pyacestep/ui/gradio/events/generation/external_lm_setup_test.pyacestep/ui/gradio/events/generation/service_init.pyacestep/ui/gradio/events/generation/service_init_helpers.pyacestep/ui/gradio/events/generation/service_init_test.pyacestep/ui/gradio/events/wiring/generation_service_wiring.pyacestep/ui/gradio/i18n/en.jsonacestep/ui/gradio/i18n/he.jsonacestep/ui/gradio/i18n/ja.jsonacestep/ui/gradio/i18n/zh.jsonacestep/ui/gradio/interfaces/generation_service_config_toggles.py
✅ Files skipped from review due to trivial changes (3)
- acestep/ui/gradio/i18n/zh.json
- acestep/ui/gradio/i18n/ja.json
- acestep/ui/gradio/i18n/en.json
🚧 Files skipped from review as they are similar to previous changes (3)
- acestep/ui/gradio/i18n/he.json
- acestep/ui/gradio/interfaces/generation_service_config_toggles.py
- acestep/ui/gradio/events/generation/service_init_test.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds the external LM vertical slice required to make the new Gradio UI flow operational end to end.
TLDR:-
Use popular local and cloud based LLM APIs to enhance Captions
Caption input
Tropical funk, female lyrics
Is enhanced to
An upbeat and breezy instrumental piece with a distinct Latin and tropical feel. The track is built upon a foundation of lively hand percussion, including bongos and shakers, creating a steady, danceable groove. A nylon-string acoustic guitar provides the core harmonic and melodic structure with rhythmic chords and arpeggios. A warm, round bass guitar underpins the arrangement. The song evolves with the introduction of a clean-toned electric guitar playing melodic fills, followed by a bright, airy flute melody that soars over the top. The arrangement is dynamic, featuring sections where the instruments layer to create a full, celebratory soundscape before gracefully fading out on the main guitar and percussion theme.
Quickstart
Open Settings and configure External LM Configuration with your provider, model, base URL, and API key (you can leave this blank for Ollama). You can use Fetch Models, Save Settings, Test Endpoint, and Runtime Doctor to confirm the setup.
For this slice you will still need to pick a 5Hz LM for planning and lyric handling.
Initialize the service as usual
Once configured, the external model is used for the caption-enhancement actions in the UI. It does not replace ACE-Step’s built-in 5Hz LM for core music generation, and it does not change the local model you pick for the main pipeline. (this will come soon)
The science:-
It introduces external LM configuration/setup UI, provider-scoped runtime hydration, and an external text-model backed Enhance Caption / format path while preserving the existing local 5Hz LM path for audio-code generation.
This PR is larger than ideal, but that is intentional for this feature slice: the UI changes only become meaningful once the setup flow, handler wiring, runtime state, and caption-enhancement path all land together. To keep the slice as small as possible, additional best-practice cleanup, deeper hardening, and follow-on polish have been deferred to later PRs.
What Changed
External LM Configurationaccordion above LM generation controls.backend=external.en/ja/zh/he.Behavioral Parity / Intentional Boundaries
Validation
Executed:
uv run python -m unittest acestep.ui.gradio.events.generation.external_lm_setup_test acestep.ui.gradio.events.generation_handlers_test acestep.ui.gradio.events.wiring.generation_service_wiring_test acestep.text_tasks.external_lm_format_client_test acestep.llm_inference_external_format_routing_testRan 41 tests ... OKpython3 -m py_compileon touched Python modules/tmp/ace-step-qa-mainScope / Out of Scope
In scope:
Out of scope:
Reviewer Focus
backend=external.CodeRabbit Scope Guard
Please treat findings as in-scope only when introduced by this external LM vertical slice.
Manual UI Validation
Important note on API key security.
Security implementation:-
saved API keys can go through the secure storage path rather than plain runtime settings
macOS/Windows use native keyring when available
Linux uses encrypted file storage with passphrase-store integration
runtime calls resolve credentials from secure/env sources instead of requiring plaintext persistence
What is not fully hardened yet:
An inline API key can still live in process memory in llm_handler.external_config after Initialize, by design, so the user can test immediately without first saving credentials
That is not disk persistence, but it is still plaintext in memory for the session
The slice focuses on getting the vertical feature working; stronger in-memory handling and stricter secret flow Cleanup were deferred
This is secure enough for a vertical-slice feature PR
But not yet the final security posture I’d want long-term, this will be addressed in the next PR
Summary by CodeRabbit
New Features
Behavior Changes
Content/Prompts
Tests
Localization