Reorder code #388
Reorder code #388
Conversation
|
Review these changes at https://app.gitnotebooks.com/AlphaSphereDotAI/chattr/pull/388 |
Reviewer's GuideThis PR reorganizes the codebase into an Class diagram for renamed App class (formerly Graph)classDiagram
class App {
+Settings settings
}
App --|> "Main application class for the Chattr Multi-agent system app"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughRefactors App into a class-centered async factory with classmethods for graph/memory/LLM setup and a public Gradio entry (App.gui()); makes generate_response a classmethod with streaming, URL/M3U8 audio download, and enhanced error handling; updates settings, logging, CI, Docker, assets, and editor tooling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant GUI as App.gui() (Gradio)
participant App as App (class)
participant Memory as cls._setup_memory()
participant LLM as cls._setup_llm()
participant Graph as cls._setup_graph()
User->>GUI: open interface
GUI->>App: App.create(settings)
App->>Memory: cls._setup_memory()
Memory-->>App: memory ready
App->>LLM: cls._setup_llm()
LLM-->>App: llm ready
App->>Graph: cls._setup_graph()
Graph-->>App: graph compiled
GUI-->>User: interface ready
User->>GUI: send message
GUI->>App: cls.generate_response(message, history)
App->>Graph: invoke compiled graph (stream)
Graph->>LLM: request tokens / stream
LLM-->>Graph: token chunks
Graph-->>App: yield (chunk, updated_history, audio_path?, video_path?)
App-->>GUI: stream updates to user
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (1)src/chattr/app/builder.py (2)
🪛 Ruff (0.14.1)src/chattr/app/builder.py83-83: Do not catch blind exception: (BLE001) 205-205: Do not catch blind exception: (BLE001) 462-462: Consider moving this statement to an (TRY300) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (1)
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 |
Summary of ChangesHello @MH0386, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a significant refactoring of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull Request Overview
Refactors the codebase by moving components from graph module to app module and reorganizing class structure. The change consolidates the application's orchestration under a unified app namespace.
- Moved
Graphclass toAppclass in theappmodule - Relocated runner functionality from
graph/runner.pytoapp/runner.py - Updated import paths throughout the codebase to reflect the new module structure
Reviewed Changes
Copilot reviewed 6 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/chattr/graph/runner.py | Removed entire file content as part of module reorganization |
| src/chattr/app/utils.py | Updated import path for logger from settings module |
| src/chattr/app/settings.py | Reorganized class definitions and improved DirectorySettings implementation |
| src/chattr/app/runner.py | New file with app initialization logic moved from graph module |
| src/chattr/app/gui.py | Updated imports and references from graph to app module |
| src/chattr/app/builder.py | Renamed Graph class to App and updated related imports |
Files not reviewed (2)
- .idea/.gitignore: Language not supported
- .idea/ruff.xml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Here's the code health analysis summary for commits Analysis Summary
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the project structure by moving several modules into a new chattr.app package. The changes are well-organized and improve the project layout. I've found a couple of areas for improvement: one is a potential issue with how directory paths are initialized in the settings, and the other is a more architectural concern about running asynchronous code at the module level, which could cause issues in different environments. My detailed feedback is in the comments below.
| from chattr.app.settings import Settings | ||
|
|
||
| settings: Settings = Settings() | ||
| app: App = run(App.create(settings)) |
There was a problem hiding this comment.
Running asyncio.run() at the module level is generally discouraged. It blocks the import process until App.create() completes, which can be slow. More importantly, it can cause conflicts if this module is imported into a larger application that already has a running asyncio event loop (e.g., a web server), as asyncio.run() creates a new event loop. This can lead to a RuntimeError: This event loop is already running. Consider using a lazy initialization pattern or an explicit initialization function that is called from your main application entry point to avoid running async code at import time.
| assets: DirectoryPath = Path.cwd() / "assets" | ||
| log: DirectoryPath = Path.cwd() / "logs" | ||
|
|
There was a problem hiding this comment.
By assigning Path.cwd() directly as a default value, the current working directory is captured when the module is first imported. This can lead to unexpected behavior if the program's working directory changes before the DirectorySettings model is instantiated. The previous implementation using default_factory was safer as it determines the path at instantiation time.
| assets: DirectoryPath = Path.cwd() / "assets" | |
| log: DirectoryPath = Path.cwd() / "logs" | |
| base: DirectoryPath = Field(default_factory=Path.cwd) | |
| assets: DirectoryPath = Field(default_factory=lambda: Path.cwd() / "assets") | |
| log: DirectoryPath = Field(default_factory=lambda: Path.cwd() / "logs") |
🔍 Vulnerabilities of
|
| digest | sha256:86056f87ac4ca9cfde7c4767e924feb71a7519d191e8d2165bde919dd1b2a34e |
| vulnerabilities |     |
| platform | linux/amd64 |
| size | 325 MB |
| packages | 500 |
# Dockerfile (28:28)
COPY --from=builder --chown=nonroot:nonroot --chmod=555 /home/nonroot/.local/ /home/nonroot/.local/
Description
| ||||||||||||
# Dockerfile (28:28)
COPY --from=builder --chown=nonroot:nonroot --chmod=555 /home/nonroot/.local/ /home/nonroot/.local/
Description
| ||||||||||||
# Dockerfile (28:28)
COPY --from=builder --chown=nonroot:nonroot --chmod=555 /home/nonroot/.local/ /home/nonroot/.local/
Description
|
🧪 CI InsightsHere's what we observed from your CI run for 1875d0f. 🟢 All jobs passed!But CI Insights is watching 👀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/chattr/app/builder.py (2)
253-265: Restoringdirectory.audiois required
DirectorySettingsno longer defines anaudiopath, but this method still callsself.settings.directory.audio. At runtimesettings.directorywon’t have that attribute, so the first tool response that hits this branch will crash withAttributeError. Reintroduce theaudio(and any other referenced) directories inDirectorySettingsbefore merging.
85-112:DirectoryPathdefaults now fail validation
The new defaults setbase,assets, andlogtoDirectoryPath, but if those folders don’t already exist Pydantic rejects the model beforecreate_missing_dirs()runs. Fresh environments (CI, new installs) will now throwValidationError. Please switch these fields toPath | None(or plainPath) withdefault_factoryand keep the existence checks in the validator.
♻️ Duplicate comments (1)
src/chattr/app/runner.py (1)
7-7: Avoid runningasyncio.run()at import time
Importing this module now executesasyncio.run(App.create(...))immediately, which will raiseRuntimeError: This event loop is already runningwhenever the importer already has an event loop (common in Gradio, notebooks, or any asyncio-based host). It also makes import slow and side-effectful. Please defer App creation to an explicit initializer (e.g.,async def init_app()plus a cached accessor) instead of callingrun()at module import.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.idea/.gitignore(1 hunks).idea/ruff.xml(1 hunks)src/chattr/app/builder.py(2 hunks)src/chattr/app/gui.py(2 hunks)src/chattr/app/runner.py(1 hunks)src/chattr/app/settings.py(2 hunks)src/chattr/app/utils.py(1 hunks)src/chattr/graph/runner.py(0 hunks)
💤 Files with no reviewable changes (1)
- src/chattr/graph/runner.py
🧰 Additional context used
🧬 Code graph analysis (3)
src/chattr/app/gui.py (1)
src/chattr/app/builder.py (1)
generate_response(201-266)
src/chattr/app/runner.py (2)
src/chattr/app/builder.py (1)
create(39-52)src/chattr/app/settings.py (1)
Settings(148-162)
src/chattr/app/builder.py (4)
src/chattr/app/settings.py (1)
Settings(148-162)src/chattr/app/state.py (1)
State(4-7)src/chattr/app/utils.py (3)
convert_audio_to_wav(61-75)download_file(33-58)is_url(13-30)src/chattr/graph/builder.py (1)
Graph(29-273)
🪛 Ruff (0.13.2)
src/chattr/app/settings.py
110-110: Do not catch blind exception: Exception
(BLE001)
140-142: Avoid specifying long messages outside the exception class
(TRY003)
144-144: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Test Image / API Test
- GitHub Check: Test Image / Docker Scout (recommendations)
- GitHub Check: Test Image / Docker Scout (quickview)
- GitHub Check: Test Image / Docker Scout (cves)
- GitHub Check: Trunk Check
- GitHub Check: Lint / Check
- GitHub Check: Sourcery review
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (1)
.idea/.gitignore (1)
9-10: LGTM—sensible IDE ignore.Adding
/copilot.data.migration.*.xmlkeeps transient Copilot migration artifacts out of version control. 👍
| url: HttpUrl = Field(default=None) | ||
| name: str = Field(default=None) | ||
| api_key: SecretStr = Field(default=None) | ||
| temperature: float = Field(default=0.0, ge=0.0, le=1.0) | ||
| system_message: str = Field( | ||
| default="You are a helpful assistant that can answer questions about the time and generate audio files from text." | ||
| ) | ||
|
|
||
| @model_validator(mode="after") | ||
| def check_api_key_exist(self) -> Self: | ||
| """ | ||
| Ensure that an API key and model name are provided if a model URL is set. | ||
| This method validates the presence of required credentials for the model provider. | ||
| Returns: | ||
| Self: The validated ModelSettings instance. | ||
| Raises: | ||
| ValueError: If the API key or model name is missing when a model URL is provided. | ||
| """ | ||
| if self.url: | ||
| if not self.api_key or not self.api_key.get_secret_value(): | ||
| raise ValueError( | ||
| "You need to provide API Key for the Model provider via `MODEL__API_KEY`" | ||
| ) | ||
| if not self.name: | ||
| raise ValueError("You need to provide Model name via `MODEL__NAME`") | ||
| return self |
There was a problem hiding this comment.
Fix optional field typing in ModelSettings
url, name, and api_key are annotated as mandatory (HttpUrl, str, SecretStr) but default to None. On instantiation Pydantic raises validation errors (“Input should be a valid URL/str”), so Settings() no longer builds in a fresh environment. Please mark these as optional (HttpUrl | None, str | None, SecretStr | None) or provide non-null defaults.
🧰 Tools
🪛 Ruff (0.13.2)
140-142: Avoid specifying long messages outside the exception class
(TRY003)
144-144: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In src/chattr/app/settings.py around lines 118 to 145, the fields url, name and
api_key are typed as non-optional but default to None which causes Pydantic
validation errors; change their type annotations to optional (e.g., HttpUrl |
None, str | None, SecretStr | None or use Optional[...] depending on project
typing style) so None is an acceptable value, and ensure any necessary typing
imports are added; keep the existing model_validator logic that enforces
presence when url is set.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
src/chattr/app/builder.py (7)
205-210: Unwrap SecretStr and handle None for ChatOpenAI parameters.
cls.settings.model.api_keyis likely aSecretStr | None, which ChatOpenAI cannot consume directly. Similarly,str(None)on an unset URL produces the literal string"None", causing connection attempts to a bogus host.Apply this diff:
try: - return ChatOpenAI( - base_url=str(cls.settings.model.url), - model=cls.settings.model.name, - api_key=cls.settings.model.api_key, - temperature=cls.settings.model.temperature, - ) + init_kwargs = { + "model": cls.settings.model.name, + "temperature": cls.settings.model.temperature, + } + if cls.settings.model.url: + init_kwargs["base_url"] = str(cls.settings.model.url) + if cls.settings.model.api_key: + init_kwargs["api_key"] = cls.settings.model.api_key.get_secret_value() + return ChatOpenAI(**init_kwargs)
250-256: Clarify the OpenAI error message.The error message is incomplete and doesn't indicate whether the API key is missing, invalid, or if there's a connectivity issue. Including the exception details will help users diagnose the problem.
Apply this diff:
except OpenAIError as e: _msg = ( - "Failed to connect to Chat Model server: " - "setting the `MODEL__API_KEY` environment variable" + "Failed to connect to Chat Model server. " + "Verify the MODEL__API_KEY environment variable is set correctly " + f"and the model URL is reachable. Error: {e}" ) logger.error(_msg) raise Error(_msg) from e
414-420: Fix audio directory path and format conversion.Two issues:
Audio directory regression:
cls.settings.directory.audiolikely doesn't exist after theDirectorySettingssimplification (per PR objectives: reduced tobase,assets,log). This will raiseAttributeErrorat runtime.Downloaded file isn't actual WAV:
_download_filestreams raw bytes to a.wavextension, but if the source is AAC, M3U8, or another format, Gradio will fail to play it.Apply this diff:
if cls._is_url(last_tool_message.content): logger.info(f"Downloading audio from {last_tool_message.content}") - file_path: Path = ( - cls.settings.directory.audio / last_tool_message.id - ) - audio_file = file_path.with_suffix(".wav") - cls._download_file(last_tool_message.content, audio_file) - logger.info(f"Audio downloaded to {audio_file}") + audio_dir = cls.settings.directory.assets / "audio" + audio_dir.mkdir(parents=True, exist_ok=True) + file_path = audio_dir / last_tool_message.id + # Download to original format first + temp_file = file_path.with_suffix(".aac") + cls._download_file(last_tool_message.content, temp_file) + # Convert to WAV for Gradio + audio_file = file_path.with_suffix(".wav") + cls._convert_audio_to_wav(temp_file, audio_file) + temp_file.unlink() # Clean up temp file + logger.info(f"Audio converted to {audio_file}")Note: You'll need to restore the
_convert_audio_to_wavmethod that was removed, or use a similar conversion approach.
464-466: Avoid variable shadowing for URL parameter.The parameter
url(typeHttpUrl) is reassigned to astrat line 466, causing type confusion and shadowing the original parameter.Apply this diff:
- if str(url).endswith(".m3u8"): - _playlist: M3U8 = load(url) - url: str = str(url).replace("playlist.m3u8", _playlist.segments[0].uri) - logger.info(f"Downloading {url} to {path}") + download_url = str(url) + if download_url.endswith(".m3u8"): + _playlist: M3U8 = load(url) + download_url = download_url.replace("playlist.m3u8", _playlist.segments[0].uri) + logger.info(f"Downloading {download_url} to {path}") session = Session() - response = session.get(url, stream=True, timeout=30) + response = session.get(download_url, stream=True, timeout=30)
422-425: Use warning instead of raising for unexpected response types.The state graph may legitimately produce other response types during streaming. Raising an
Errorwill crash the chat and provide a poor user experience. Logging a warning and continuing is more appropriate.Apply this diff:
else: - _msg = f"Unsupported audio source: {response.keys()}" + _msg = f"Unsupported response type: {response.keys()}" logger.warning(_msg) - raise Error(_msg) + continue
464-475: Add error handling for M3U8 playlist processing.The M3U8 playlist handling lacks error handling and uses fragile string replacement logic. The playlist may fail to load, contain no segments, or have a different URL structure than expected.
Apply this diff:
+ from urllib.parse import urljoin + download_url = str(url) if download_url.endswith(".m3u8"): - _playlist: M3U8 = load(url) - download_url = download_url.replace("playlist.m3u8", _playlist.segments[0].uri) + try: + _playlist: M3U8 = load(url) + if not _playlist.segments: + raise ValueError("M3U8 playlist contains no segments") + # Resolve segment URI relative to playlist base + base_url = download_url.rsplit('/', 1)[0] + '/' + download_url = urljoin(base_url, _playlist.segments[0].uri) + except Exception as e: + _msg = f"Failed to process M3U8 playlist: {e}" + logger.error(_msg) + raise Error(_msg) from e logger.info(f"Downloading {download_url} to {path}")
421-426: Restructure to avoid duplicate yields.Line 426 always executes after the conditional yields in lines 421. This causes redundant yields after audio generation. The generator should yield once per response update.
Consider restructuring with a single yield point:
if cls._is_url(last_tool_message.content): logger.info(f"Downloading audio from {last_tool_message.content}") # ... audio download logic ... is_audio_generated = True - yield "", history, audio_file, None else: _msg = f"Unsupported response type: {response.keys()}" logger.warning(_msg) continue yield "", history, audio_file if is_audio_generated else None, NoneOr use
continueafter the audio yield:is_audio_generated = True yield "", history, audio_file, None + continue
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
pyproject.toml(2 hunks)src/chattr/app/builder.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/chattr/app/builder.py (2)
src/chattr/app/settings.py (1)
Settings(147-162)src/chattr/app/state.py (1)
State(4-7)
🪛 Ruff (0.14.0)
src/chattr/app/builder.py
83-83: Do not catch blind exception: Exception
(BLE001)
187-187: Do not catch blind exception: Exception
(BLE001)
444-444: Consider moving this statement to an else block
(TRY300)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Trunk Check
- GitHub Check: Lint / Check
- GitHub Check: Test Image / Build and push Docker image to ghcr.io
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (4)
pyproject.toml (4)
19-19: LGTM: poml addition is justified and version constraint is reasonable.poml 0.0.8 includes numerous issue resolutions and is marked as a final pre-release, aligning with the PR's use of POML (Prompt Orchestration Markup Language) for bringing structure and maintainability to prompt engineering.
31-31: Dev dependencies are appropriately scoped.The additions of
doppler-env(environment configuration) anduv-build(supporting theuv_buildbackend) are reasonable and correctly placed in the dev dependency group.Also applies to: 37-37
40-41: The Ruff configuration file is present and valid.The referenced
.trunk/configs/ruff.tomlfile exists and contains properly formatted Ruff configuration. Theextenddirective inpyproject.tomlis correctly configured and will not cause build failures.
12-12: The original concern does not apply to the current constraint.CVE-2023-6572 was patched in gradio 4.14.0 (or any later release), affecting only versions < 4.14.0. The constraint
gradio>=5.48.0already requires a version well above this patch threshold, meaning all versions that satisfy this constraint have the fix applied.The Docker Scout vulnerability scan flagging version 5.49.1 appears to reflect stale vulnerability data. No changes to the version constraint are needed.
Likely an incorrect or invalid review comment.
| cls._model = cls._llm.bind_tools(cls._tools, parallel_tool_calls=False) | ||
| cls._memory = await cls._setup_memory() | ||
| cls._graph = cls._setup_graph() | ||
| return cls() |
There was a problem hiding this comment.
Fix the factory return value.
Line 89 returns cls(), which creates a new instance with empty state, while all the setup work stored data in class-level attributes. This makes the returned instance unusable.
Apply this diff:
- return cls()
+ return clsAlternatively, if you intend proper instance isolation, convert all class-level attributes to instance attributes and change all cls.* references to self.* throughout the class (as previously suggested in past reviews).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return cls() | |
| return cls |
🤖 Prompt for AI Agents
In src/chattr/app/builder.py around line 89, the factory currently returns cls()
which instantiates a new object with empty instance state while setup populated
class-level attributes, so the returned object is unusable; fix by returning the
class itself (return cls) instead of instantiating it, OR if instance isolation
is required, convert all class-level attributes to instance attributes and
update every cls.* reference to self.* across the class so the factory can
continue to return an initialized instance.
Adds a new node to the state graph that proactively removes old audio and video files from temporary directories before processing a new agent request. This prevents the accumulation of unused files and helps manage disk space. Also updates the memory update call to pass relevant messages and user ID, ensuring comprehensive context is stored for the conversation.
|
Summary by Sourcery
Refactor code structure by moving core modules into the chattr.app package, reorder and simplify settings classes, and update imports accordingly.
Enhancements: