fix(api): make api_server importable without gradio installed - #1276
fix(api): make api_server importable without gradio installed#1276tsondo wants to merge 3 commits into
Conversation
The API server only needs _build_generation_info, but importing it via acestep.ui.gradio.events.results_handlers dragged in the entire gradio UI package at import time. Three changes keep the API path headless: - ui/gradio/__init__.py: expose create_gradio_interface lazily (PEP 562) - ui/gradio/events/__init__.py: move the .wiring import (gradio at module level) inside the two setup functions, which only run under the Gradio UI - api_server.py: import _build_generation_info from its defining module (results/generation_info.py, already headless-safe) instead of the facade Gradio UI behavior is unchanged when gradio is installed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughGradio-dependent imports are deferred across the API, Gradio package, and event package. Generation metadata uses its defining module, the interface factory loads lazily, and event wiring imports occur inside setup functions. ChangesHeadless import behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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
🤖 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 `@acestep/ui/gradio/__init__.py`:
- Around line 9-14: Declare create_gradio_interface in the module’s __all__ so
star imports include the lazily resolved facade export, while retaining the
existing __getattr__ implementation. Add a concise comment documenting that
__getattr__ is required to lazily expose new Python functions.
In `@acestep/ui/gradio/events/__init__.py`:
- Around line 101-107: Update the docstring for setup_training_event_handlers to
add Args entries for all four inputs and a Returns section documenting that
handlers are registered in place, while retaining its existing purpose
description.
🪄 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 Plus
Run ID: 20341226-5a3d-4021-8df1-358d9b637147
📒 Files selected for processing (3)
acestep/api_server.pyacestep/ui/gradio/__init__.pyacestep/ui/gradio/events/__init__.py
| def __getattr__(name): | ||
| if name == "create_gradio_interface": | ||
| from acestep.ui.gradio.interfaces import create_gradio_interface | ||
|
|
||
| return create_gradio_interface | ||
| raise AttributeError(f"module {__name__!r} has no attribute {name!r}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -n -U 'from\s+acestep\.ui\.gradio\s+import\s+\*' --glob '*.py' .Repository: ace-step/ACE-Step-1.5
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files | rg '(^|/)acestep/ui/gradio/__init__\.py$|gradio/'
echo
echo "== target file =="
if [ -f acestep/ui/gradio/__init__.py ]; then
nl -ba acestep/ui/gradio/__init__.py
fi
echo
echo "== imports/usages of acestep.ui.gradio =="
rg -n 'acestep\.ui\.gradio|create_gradio_interface|from acestep.ui.gradio import|import acestep.ui.gradio' --glob '*.py' .Repository: ace-step/ACE-Step-1.5
Length of output: 8511
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target file contents =="
awk '{printf "%6d\t%s\n", NR, $0}' acestep/ui/gradio/__init__.py
echo
echo "== uses of acestep.ui.gradio interface exports =="
rg -n 'acestep\.ui\.gradio|create_gradio_interface' --glob '*.py' .
echo
echo "== import forms for acestep.ui.gradio =="
rg -nU 'from\s+acestep\.ui\.gradio\s+(?:import\s+\*|import)|import\s+acestep\.ui\.gradio|from\s+acestep\.ui\s+import\s+gradio|import\s+acestep\.ui\.gradio' --glob '*.py' .
echo
echo "== any __getattr__ in acestep ui gradio =="
rg -n '__getattr__|__all__' --glob 'acestep/ui/gradio/**/*.py' --glob 'acestep/ui/gradio/__init__.py' .Repository: ace-step/ACE-Step-1.5
Length of output: 30439
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Python __getattr__ star-import behavior probe =="
python3 - <<'PY'
import ast
import sys
import tempfile
import os
from importlib.machinery import ModuleSpec
from importlib.abc import Loader, MetaPathFinder
from types import ModuleType
pkg = ModuleType("testpkg")
pkg.__file__ = "<testpkg-file>"
subpkg = ModuleType("testpkg.submod")
subpkg.__file__ = "<testpkg-submod-file>"
subpkg.__path__ = []
subpkg.__getattr__ = globals()["__getattr__"] = lambda name: "VALUE" if name == "x" else None
sys.modules["testpkg"] = pkg
sys.modules["testpkg.submod"] = subpkg
ns = {}
exec("from testpkg.submod import *", ns)
print("__all__", ns.get("__all__"))
print("x", ns.get("x"))
print("hasattr", hasattr(subpkg, "x"))
print("getattr", getattr(subpkg, "x", "<missing>",))
code = ast.parse("from testpkg.submod import *")
print("Star import nodes:", ast.dump(code.body))
PYRepository: ace-step/ACE-Step-1.5
Length of output: 511
🌐 Web query:
Python PEP 562 module __getattr__ from import * __all__ getattr list returned behavior
💡 Result:
PEP 562 introduced module-level getattr and dir to Python (starting in version 3.7), allowing for dynamic attribute resolution and customized introspection [1][2]. The interaction between module getattr, all, and from module import * is governed by the following rules: 1. Import * Behavior and all: The statement from module import * is controlled by the module's all attribute [3][4]. If all is defined, only the names listed in it are imported [3][4]. If all is not defined, Python imports all names in the module that do not begin with an underscore [4]. all does not implicitly call getattr for names not explicitly listed [5]. 2. Role of getattr: Module-level getattr is called only when an attribute lookup fails through the standard mechanisms (i.e., it is not found in the module's dict) [1][6][5]. It is not automatically invoked by the import * mechanism to discover or generate names [5]. 3. Interaction with from module import *: When you use from module import *, the interpreter looks at all [3]. If a name listed in all is missing from the module's namespace, the import machinery will attempt to resolve it. If the module has defined getattr, this may trigger a call to it as the system searches for the attribute [7]. 4. Important Technical Note: Developers often use getattr combined with all to implement lazy loading of submodules [1][8]. In such patterns, all lists the submodule names, and getattr performs the import only when the attribute is first accessed [1]. Because getattr is called by the attribute access mechanism, it is called only when a name is actually used, which can differ from the eager loading behavior typically associated with from module import * [1][9]. In summary, getattr does not change the fundamental behavior of all or how import * filters names [5]. To include dynamically generated names in an import * operation, you must ensure those names are present in the all list [5]. If you want to customize the names shown by dir, you should implement module-level dir [1][6].
Citations:
- 1: https://peps.python.org/pep-0562/
- 2: https://stackoverflow.com/questions/60265327/getattr-on-module-doesnt-behave-the-way-the-pep-suggested
- 3: https://realpython.com/python-all-attribute/
- 4: https://stackoverflow.com/questions/44834/what-does-all-mean-in-python
- 5: https://runebook.dev/en/docs/python/reference/datamodel/module.__dir__
- 6: https://docs.python.org/3/reference/datamodel.html
- 7: https://stackoverflow.com/questions/75823000/modules-getattr-is-called-twice
- 8: https://mail.python.org/pipermail/python-dev/2017-November/150542.html
- 9: Implement PEP 562: module __getattr__ and __dir__ python/cpython#76406
Preserve star-import compatibility for the facade export.
Without __all__, from acestep.ui.gradio import * currently imports names defined directly in the module namespace and ignores lazy names resolved through __getattr__, so create_gradio_interface is not included. Declare it in __all__ if this facade is meant to support star imports, and document the lazy __getattr__ hook as required for new Python functions.
🤖 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 `@acestep/ui/gradio/__init__.py` around lines 9 - 14, Declare
create_gradio_interface in the module’s __all__ so star imports include the
lazily resolved facade export, while retaining the existing __getattr__
implementation. Add a concise comment documenting that __getattr__ is required
to lazily expose new Python functions.
Source: Coding guidelines
|
This is the right direction for keeping
Once those are in, I can do a focused final pass and merge it. |
- Declare __all__ (plus a matching __dir__) on acestep.ui.gradio so the lazily exposed create_gradio_interface stays visible to star imports and dir() under PEP 562. - Expand the setup_training_event_handlers docstring with Args/Returns, mirroring setup_event_handlers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MGYzKnqg6ti5DENrxcfwnp
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MGYzKnqg6ti5DENrxcfwnp
3 participants
Summary
acestep.api_serveronly needs one helper from the UI layer (_build_generation_info), but importing it viaacestep.ui.gradio.events.results_handlersdrags in the entire gradio UI package at import time — so the API server cannot start in environments where gradio is not installed (headless / API-only deployments that consume ACE-Step purely through REST).Three changes keep the API path headless while leaving Gradio UI behavior unchanged when gradio is installed:
acestep/ui/gradio/__init__.py— exposecreate_gradio_interfacelazily (PEP 562__getattr__) instead of importing the interfaces package eagerlyacestep/ui/gradio/events/__init__.py— move the.wiringimport (whose handler modules import gradio at module level) inside the two setup functions, which only run under the Gradio UIacestep/api_server.py— import_build_generation_infofrom its defining module (events/results/generation_info.py, already headless-safe with its own lazy gradio fallback) instead of theresults_handlersfacadeTesting
import acestep.api_serversucceeds with gradio imports blocked (simulating an environment without gradio)from acestep.ui.gradio import create_gradio_interfaceandfrom acestep.ui.gradio.events import setup_event_handlers, setup_training_event_handlersstill work with gradio installedresults_handlers_facade_test.pyandi18n_thread_safety_test.pypassRelated: #1275 declares
python-multipartdirectly — headless installs need it since it currently only arrives via gradio.🤖 Generated with Claude Code
Summary by CodeRabbit