feat: 0.1.0b2 — hardware API, SSE pull, TUI auto-refresh, Windows polish#5
Merged
Conversation
…indows polish Add quality-of-life and reliability upgrades for the 0.1.0b2 public beta: API & Web UI: - GET /v1/hardware returns hardware profile JSON for the Web UI badge - POST /v1/pull streams pull progress over SSE so the Web UI can show real download/quantization output instead of a blocking spinner - Web UI badge now reflects the live GPU + effective memory budget - Web UI Pull button consumes the SSE stream and refreshes models on DONE CLI & Cross-platform: - Fix opendrop run hang/crash on Windows by replacing threading.Event().wait() with a portable while-sleep + KeyboardInterrupt loop - opendrop list --search/-s filters the registry by case-insensitive substring on id and display_name - Registry.search_models() supports the same filter at the data layer - llama-server discovery now finds llama-server.exe and Windows install paths (Program Files\llama.cpp, ~/llama.cpp/build/bin[/Release]) - require_server_binary() produces platform-specific install hints (Windows release link, macOS Homebrew, Linux build instructions) - LlamaCppServer.stop() uses TerminateProcess on Windows (no SIGTERM) TUI: - 5-second auto-refresh via Textual set_interval keeps the model table and running-state in sync without exiting and restarting the app - "r" binding now triggers a manual force-refresh instead of restart Release plumbing: - .github/workflows/publish.yml uses PyPI Trusted Publishing (OIDC) on v*.*.* and v*.*.*b* tags - CHANGELOG bumped to [0.1.0b2] - 2026-05-14 with all of the above Tests: - +11 new tests (108 → 119 passing): /v1/hardware shape, /v1/pull validation and SSE content-type, Registry.search_models behavior, list --search CLI - Updated test_run_flow to patch time.sleep instead of threading.Event.wait
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR implements version 0.1.0b2 with model search functionality, hardware and model pull REST endpoints, web UI streaming integration, TUI auto-refresh, Windows cross-platform improvements, and automated PyPI publishing. The changes span CLI, core registry, inference server, both UI frontends, test coverage, and release infrastructure. Changesv0.1.0b2 Feature Release
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3a0ca8883
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+6
to
+7
| - "v[0-9]+.[0-9]+.[0-9]+" | ||
| - "v[0-9]+.[0-9]+.[0-9]+b[0-9]+" |
There was a problem hiding this comment.
The push.tags entries are written like regexes, but GitHub Actions evaluates these as glob patterns, so + is treated literally; tags such as v0.1.0 or v0.1.0b2 will not match these filters and the publish workflow will never trigger on normal release tags. This blocks the intended PyPI release path for every new version tag.
Useful? React with 👍 / 👎.
Comment on lines
+214
to
+216
| original_console = _orch_mod.console | ||
| _orch_mod.console = _QueuingConsole() # type: ignore[assignment] | ||
| try: |
There was a problem hiding this comment.
This endpoint mutates opendrop.core.orchestrator.console globally per request, which is unsafe when two /v1/pull requests run concurrently in executor threads: one request can restore the global while the other is still pulling, causing progress to leak to the wrong sink or stop streaming, and the final restore can leave a stale request-scoped console installed for future pulls. The race is introduced by assigning module-global state in request handling.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
.github/workflows/publish.yml (1)
18-19: ⚡ Quick winPin GitHub Actions to immutable commit SHAs.
Using mutable version tags (
@v4,@v5,@release/v1) weakens supply-chain security because upstream tags can move or be overwritten. Replace with full 40-character commit SHAs for eachuses:line: lines 18, 19, 27, 41, and 46.🤖 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 @.github/workflows/publish.yml around lines 18 - 19, The workflow currently references mutable action tags like actions/checkout@v4 and actions/setup-python@v5 (and other uses: entries at the same places for release/v1 etc. noted in the review); replace each mutable tag with the corresponding full 40-character commit SHA for that action (e.g., actions/checkout@<full-sha>, actions/setup-python@<full-sha>, and the other uses entries) so every uses: line is pinned to an immutable commit SHA to harden the supply chain.tests/test_server.py (1)
209-223: ⚡ Quick winOptional: Verify SSE streaming content, not just headers.
The test confirms the endpoint returns
text/event-streambut doesn't verify that actual progress lines or the[DONE]sentinel are streamed correctly. Consider reading the response body to assert expected SSE behavior.🧪 Example: Verify SSE content
def test_pull_streams_sse(self, client: TestClient, monkeypatch): import opendrop.inference.server as server_mod class _FakeOrch: def pull(self, source, token=None, quant_override=None, **_kw): pass # no-op, sentinel will fire via finally monkeypatch.setattr(server_mod, "Orchestrator", _FakeOrch) r = client.post( "/v1/pull", json={"source": "org/model"}, headers={"Accept": "text/event-stream"}, ) assert r.status_code == 200 assert "text/event-stream" in r.headers.get("content-type", "") # Verify streaming content content = r.text assert "data: [DONE]" in content🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_server.py` around lines 209 - 223, Update the test_pull_streams_sse to also verify the SSE body content: after making the POST request (keeping the monkeypatched Orchestrator/_FakeOrch and pull), read the response body (r.text or iterate r.iter_lines()) and assert it contains the SSE sentinel "data: [DONE]" and optionally a progress/data line; this ensures the Orchestrator.pull streaming behavior is validated in addition to the content-type header.opendrop/ui/web.py (1)
224-228: ⚡ Quick winParse JSON error detail from FastAPI 422 response.
Line 226 reads the error response as plain text, but FastAPI returns 422 validation errors as JSON with a
detailfield. Parsing this would show the user a more precise error message.📝 Proposed improvement
if (!resp.ok) { - const err = await resp.text(); - addSystemMsg(`Pull failed: ${err}`); + let errMsg = 'Unknown error'; + try { + const errData = await resp.json(); + errMsg = errData.detail || errData.message || JSON.stringify(errData); + } catch { + errMsg = await resp.text(); + } + addSystemMsg(`Pull failed: ${errMsg}`); return; }🤖 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 `@opendrop/ui/web.py` around lines 224 - 228, The error handling for fetch responses currently reads resp.text() and shows a raw string; change it to attempt JSON parsing of resp (e.g., await resp.json()) and extract the FastAPI validation message from the "detail" field (falling back to a readable summary or to the plain text if JSON parsing fails), then pass that extracted message into addSystemMsg; update the code paths around resp, err, and addSystemMsg to use the parsed detail when available.opendrop/inference/server.py (1)
181-189: ⚡ Quick winConsider using Pydantic for request validation.
The endpoint manually parses the JSON body and validates the
sourcefield. FastAPI best practice is to define a Pydantic model for request validation, which provides automatic validation, error messages, and OpenAPI schema generation.📦 Proposed Pydantic model approach
Define a request model at the top of the file alongside other models:
class PullRequest(BaseModel): source: str token: str | None = None quant: str | None = NoneThen update the endpoint signature:
- async def pull_model(request: Request) -> StreamingResponse: - body = await request.json() - source: str = body.get("source", "") - token: str | None = body.get("token", None) - quant: str | None = body.get("quant", None) - - if not source: - raise HTTPException(status_code=422, detail="'source' is required") + async def pull_model(req: PullRequest) -> StreamingResponse: + source = req.source + token = req.token + quant = req.quant🤖 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 `@opendrop/inference/server.py` around lines 181 - 189, The pull endpoint currently reads and validates the request JSON manually inside pull_model; define a Pydantic model (e.g., class PullRequest(BaseModel) with source: str, token: Optional[str]=None, quant: Optional[str]=None), add the BaseModel import, and change the route signature from async def pull_model(request: Request) to async def pull_model(payload: PullRequest) so FastAPI performs validation and automatic 422 responses; then replace body.get(...) usages with payload.source, payload.token, payload.quant and remove the manual "'source' is required" check.tests/test_cli.py (1)
250-263: ⚡ Quick winStrengthen the no-match test to exercise filtering, not just empty registry.
This test currently returns an empty registry, so it can still pass if search filtering regresses.
✅ Suggested test adjustment
monkeypatch.setattr( registry_mod, "Registry", - lambda *_a, **_kw: SimpleNamespace(list_models=lambda: []), + lambda *_a, **_kw: SimpleNamespace( + list_models=lambda: [ + SimpleNamespace(id="mistral-7b", display_name="Mistral 7B") + ] + ), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cli.py` around lines 250 - 263, The test test_list_search_no_match_shows_empty_message currently fakes Registry.list_models to return an empty list so it doesn't verify search filtering; change the monkeypatch for registry_mod.Registry so list_models returns a non-empty list of model entries (e.g., objects or dicts with names/ids) that purposely do NOT match the search term you're testing, then run the same CLI/list-search code path so the test asserts the "no results" message is produced due to filtering (not an empty registry). Ensure you update the mocked values returned by Registry.list_models and keep the test invoking the same search logic so regressions in the filter will fail.tests/test_registry.py (1)
179-198: ⚡ Quick winBroaden search tests to cover all advertised columns and isolate
display_name.Current coverage can pass without proving
display_name-specific matching (it mirrorsid) and doesn’t validatemodel_id/architecture/pipeline_tagbranches.🧪 Suggested additions
def test_search_models_by_display_name(self, registry: Registry): - registry.add_model(_make_record("-z")) - results = registry.search_models("llama-3-8b-q4-km-z") + rec = _make_record("-z") + rec.display_name = "Friendly Llama Z" + registry.add_model(rec) + results = registry.search_models("friendly llama") assert len(results) == 1 + def test_search_models_by_model_metadata_fields(self, registry: Registry): + rec = _make_record("-meta") + rec.model_id = "org/special-model" + rec.architecture = "mixtral" + rec.pipeline_tag = "text2sql" + registry.add_model(rec) + assert registry.search_models("special-model") + assert registry.search_models("mixtral") + assert registry.search_models("text2sql")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_registry.py` around lines 179 - 198, The current tests (test_search_models_by_display_name, test_search_models_case_insensitive, test_search_models_no_match, test_search_models_partial_match) only use _make_record variants that keep display_name identical to id so they don't prove display_name-specific matching; update and add tests that create Registry records where display_name differs from id (e.g., id="llama-x", display_name="LLaMA Special") and assert that Registry.search_models matches on display_name (case-insensitive), and also add separate tests that target the other search branches by creating records with unique model_id, architecture, and pipeline_tag values and asserting search_models returns those records when queried for those fields; refer to Registry.search_models, display_name, model_id, architecture, and pipeline_tag when locating code to modify.opendrop/ui/tui.py (1)
200-214: ⚡ Quick winExtract duplicated row-building logic into a helper method.
The row-building logic here duplicates the identical logic in
ModelTable.on_mount()(lines 84-98). Extract this into a shared helper method to maintain a single source of truth.♻️ Proposed refactor
Add a helper method to
ModelTable:def populate_rows(self, records: list[ModelRecord], running_ids: set[str]) -> None: """Populate table with model records and status indicators.""" for rec in records: status = ( Text("● running", style="bold green") if rec.id in running_ids else Text("○ idle", style="dim") ) self.add_row( rec.id, rec.display_name, rec.architecture or "—", f"{rec.params_b:.1f}B" if rec.params_b else "—", rec.quant, rec.size_human(), status, )Then update
ModelTable.on_mount():def on_mount(self) -> None: self.add_columns(*self.COLUMNS) - for rec in self._records: - status = ( - Text("● running", style="bold green") - if rec.id in self._running_ids - else Text("○ idle", style="dim") - ) - self.add_row( - rec.id, - rec.display_name, - rec.architecture or "—", - f"{rec.params_b:.1f}B" if rec.params_b else "—", - rec.quant, - rec.size_human(), - status, - ) + self.populate_rows(self._records, self._running_ids)And simplify
_refresh_models_table():async def _refresh_models_table(self) -> None: try: records = self._model_registry.list_models() running = set(get_manager().running_models().keys()) table: ModelTable = self.query_one(ModelTable) table.clear() - for rec in records: - status = ( - Text("● running", style="bold green") - if rec.id in running - else Text("○ idle", style="dim") - ) - table.add_row( - rec.id, - rec.display_name, - rec.architecture or "—", - f"{rec.params_b:.1f}B" if rec.params_b else "—", - rec.quant, - rec.size_human(), - status, - ) + table.populate_rows(records, running) except Exception: pass🤖 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 `@opendrop/ui/tui.py` around lines 200 - 214, Extract the duplicated row-building logic into a new helper method on ModelTable (e.g., def populate_rows(self, records: list[ModelRecord], running_ids: set[str]) -> None) that builds the status Text and calls self.add_row(...) for each rec (use rec.id, rec.display_name, rec.architecture or "—", f"{rec.params_b:.1f}B" if rec.params_b else "—", rec.quant, rec.size_human(), status); then replace the row-construction loop in ModelTable.on_mount() and the loop in _refresh_models_table() to call self.populate_rows(records, running) (or appropriate local names) so both sites share the single implementation.
🤖 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 `@opendrop/cli.py`:
- Around line 270-276: The empty-state message is misleading when a search
filter is applied; update the branch after filtering (where search, q, and
records are used) to print a different message when search is truthy—use
console.print to show something like "No models match '<search>'" (or "No models
in registry" when search is empty) so that the code path using search, q,
records, and console.print reports the correct empty-state.
In `@opendrop/inference/llamacpp.py`:
- Around line 30-31: The _SERVER_NAMES list contains a generic "server" entry
that can match unrelated PATH executables; remove the generic "server" (and any
non-llama generic variants) from _SERVER_NAMES so only llama-specific names
remain (e.g., "llama-server", "llama_server", "llama-server.exe"); update any
dependent binary-discovery logic that reads _SERVER_NAMES to rely on the
narrowed list (no functional code changes beyond the constant).
In `@opendrop/inference/server.py`:
- Around line 224-231: Assign and track the background executor Future returned
by loop.run_in_executor when starting _run_pull (e.g., store it in a variable
like pull_future) and wrap the SSE loop in try/finally so you can cancel and
await that Future when the client disconnects; specifically, in _event_stream
obtain pull_future = loop.run_in_executor(None, _run_pull), then in the finally
block if not pull_future.done() call pull_future.cancel() and await it (handling
asyncio.CancelledError/other exceptions), and also ensure you signal the
synchronous _run_pull to stop further queue.put()s (for example by putting None
or checking a shared flag) so no orphaned queue operations occur.
- Around line 211-222: The code currently monkey-patches the module-level
opendrop.core.orchestrator.console which causes races; refactor
Orchestrator.pull to accept an optional console parameter (e.g., def pull(self,
source, token=None, quant_override=None, console=None)) and inside pull use a
local _console = console or opendrop.core.orchestrator.console everywhere
instead of the module global; then update the server handler to instantiate
_QueuingConsole and pass it as the console argument to orch.pull rather than
assigning to _orch_mod.console and remove the try/finally monkey-patch block so
each request uses its own console instance and no shared global is mutated.
In `@opendrop/ui/tui.py`:
- Around line 215-216: Replace the bare "except Exception: pass" in
opendrop/ui/tui.py with proper logging: catch the exception as "except Exception
as e" and call a logger (e.g., logger.exception(...) or logging.exception(...))
to record the stack trace and context (import logging or use the module/class
logger already defined in this file); keep the swallow behavior only if
intentional—otherwise consider re-raising or handling specific exceptions in the
surrounding refresh/loop function that contains the current except block.
In `@opendrop/ui/web.py`:
- Around line 185-196: In loadHardware, guard against missing or malformed hw
fields before using them: check that the fetched hw object exists, that
hw.effective_memory_mb is a finite number (fallback to 0 or null) before
dividing and calling toFixed, and that hw.gpu_name or hw.gpu_kind are present
before building gpuLabel; update the logic that computes memGB and gpuLabel (and
the assignment to document.getElementById('hw-badge').textContent) to use safe
defaults so missing fields do not throw and the catch only triggers for
network/errors.
---
Nitpick comments:
In @.github/workflows/publish.yml:
- Around line 18-19: The workflow currently references mutable action tags like
actions/checkout@v4 and actions/setup-python@v5 (and other uses: entries at the
same places for release/v1 etc. noted in the review); replace each mutable tag
with the corresponding full 40-character commit SHA for that action (e.g.,
actions/checkout@<full-sha>, actions/setup-python@<full-sha>, and the other uses
entries) so every uses: line is pinned to an immutable commit SHA to harden the
supply chain.
In `@opendrop/inference/server.py`:
- Around line 181-189: The pull endpoint currently reads and validates the
request JSON manually inside pull_model; define a Pydantic model (e.g., class
PullRequest(BaseModel) with source: str, token: Optional[str]=None, quant:
Optional[str]=None), add the BaseModel import, and change the route signature
from async def pull_model(request: Request) to async def pull_model(payload:
PullRequest) so FastAPI performs validation and automatic 422 responses; then
replace body.get(...) usages with payload.source, payload.token, payload.quant
and remove the manual "'source' is required" check.
In `@opendrop/ui/tui.py`:
- Around line 200-214: Extract the duplicated row-building logic into a new
helper method on ModelTable (e.g., def populate_rows(self, records:
list[ModelRecord], running_ids: set[str]) -> None) that builds the status Text
and calls self.add_row(...) for each rec (use rec.id, rec.display_name,
rec.architecture or "—", f"{rec.params_b:.1f}B" if rec.params_b else "—",
rec.quant, rec.size_human(), status); then replace the row-construction loop in
ModelTable.on_mount() and the loop in _refresh_models_table() to call
self.populate_rows(records, running) (or appropriate local names) so both sites
share the single implementation.
In `@opendrop/ui/web.py`:
- Around line 224-228: The error handling for fetch responses currently reads
resp.text() and shows a raw string; change it to attempt JSON parsing of resp
(e.g., await resp.json()) and extract the FastAPI validation message from the
"detail" field (falling back to a readable summary or to the plain text if JSON
parsing fails), then pass that extracted message into addSystemMsg; update the
code paths around resp, err, and addSystemMsg to use the parsed detail when
available.
In `@tests/test_cli.py`:
- Around line 250-263: The test test_list_search_no_match_shows_empty_message
currently fakes Registry.list_models to return an empty list so it doesn't
verify search filtering; change the monkeypatch for registry_mod.Registry so
list_models returns a non-empty list of model entries (e.g., objects or dicts
with names/ids) that purposely do NOT match the search term you're testing, then
run the same CLI/list-search code path so the test asserts the "no results"
message is produced due to filtering (not an empty registry). Ensure you update
the mocked values returned by Registry.list_models and keep the test invoking
the same search logic so regressions in the filter will fail.
In `@tests/test_registry.py`:
- Around line 179-198: The current tests (test_search_models_by_display_name,
test_search_models_case_insensitive, test_search_models_no_match,
test_search_models_partial_match) only use _make_record variants that keep
display_name identical to id so they don't prove display_name-specific matching;
update and add tests that create Registry records where display_name differs
from id (e.g., id="llama-x", display_name="LLaMA Special") and assert that
Registry.search_models matches on display_name (case-insensitive), and also add
separate tests that target the other search branches by creating records with
unique model_id, architecture, and pipeline_tag values and asserting
search_models returns those records when queried for those fields; refer to
Registry.search_models, display_name, model_id, architecture, and pipeline_tag
when locating code to modify.
In `@tests/test_server.py`:
- Around line 209-223: Update the test_pull_streams_sse to also verify the SSE
body content: after making the POST request (keeping the monkeypatched
Orchestrator/_FakeOrch and pull), read the response body (r.text or iterate
r.iter_lines()) and assert it contains the SSE sentinel "data: [DONE]" and
optionally a progress/data line; this ensures the Orchestrator.pull streaming
behavior is validated in addition to the content-type header.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9de4a21e-52ac-48fe-b2f5-a0e52fbe4f9a
📒 Files selected for processing (11)
.github/workflows/publish.ymlCHANGELOG.mdopendrop/cli.pyopendrop/core/registry.pyopendrop/inference/llamacpp.pyopendrop/inference/server.pyopendrop/ui/tui.pyopendrop/ui/web.pytests/test_cli.pytests/test_registry.pytests/test_server.py
Comment on lines
+270
to
276
| if search: | ||
| q = search.lower() | ||
| records = [r for r in records if q in r.id.lower() or q in r.display_name.lower()] | ||
|
|
||
| if not records: | ||
| console.print("[dim]No models in registry. Run `opendrop pull <url>` to get started.[/dim]") | ||
| return |
There was a problem hiding this comment.
Fix misleading empty-state text for --search.
When a filter is applied, Line 275 still says “No models in registry,” which is incorrect if models exist but none match.
💡 Suggested patch
- if not records:
- console.print("[dim]No models in registry. Run `opendrop pull <url>` to get started.[/dim]")
- return
+ if not records:
+ if search:
+ console.print(f"[dim]No models match '{search}'.[/dim]")
+ else:
+ console.print("[dim]No models in registry. Run `opendrop pull <url>` to get started.[/dim]")
+ return📝 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.
Suggested change
| if search: | |
| q = search.lower() | |
| records = [r for r in records if q in r.id.lower() or q in r.display_name.lower()] | |
| if not records: | |
| console.print("[dim]No models in registry. Run `opendrop pull <url>` to get started.[/dim]") | |
| return | |
| if search: | |
| q = search.lower() | |
| records = [r for r in records if q in r.id.lower() or q in r.display_name.lower()] | |
| if not records: | |
| if search: | |
| console.print(f"[dim]No models match '{search}'.[/dim]") | |
| else: | |
| console.print("[dim]No models in registry. Run `opendrop pull <url>` to get started.[/dim]") | |
| return |
🤖 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 `@opendrop/cli.py` around lines 270 - 276, The empty-state message is
misleading when a search filter is applied; update the branch after filtering
(where search, q, and records are used) to print a different message when search
is truthy—use console.print to show something like "No models match '<search>'"
(or "No models in registry" when search is empty) so that the code path using
search, q, records, and console.print reports the correct empty-state.
Comment on lines
+30
to
+31
| _SERVER_NAMES = ["llama-server", "llama_server", "server", "llama-server.exe"] | ||
| _QUANTIZE_NAMES = ["llama-quantize", "llama_quantize", "llama-quantize.exe"] |
There was a problem hiding this comment.
Avoid generic server fallback in binary discovery.
Line 30 includes "server" in _SERVER_NAMES, which can resolve an unrelated executable from PATH and start the wrong process. Restrict matching to llama-specific names only.
Suggested patch
-_SERVER_NAMES = ["llama-server", "llama_server", "server", "llama-server.exe"]
+_SERVER_NAMES = ["llama-server", "llama_server", "llama-server.exe", "llama_server.exe"]📝 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.
Suggested change
| _SERVER_NAMES = ["llama-server", "llama_server", "server", "llama-server.exe"] | |
| _QUANTIZE_NAMES = ["llama-quantize", "llama_quantize", "llama-quantize.exe"] | |
| _SERVER_NAMES = ["llama-server", "llama_server", "llama-server.exe", "llama_server.exe"] | |
| _QUANTIZE_NAMES = ["llama-quantize", "llama_quantize", "llama-quantize.exe"] |
🤖 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 `@opendrop/inference/llamacpp.py` around lines 30 - 31, The _SERVER_NAMES list
contains a generic "server" entry that can match unrelated PATH executables;
remove the generic "server" (and any non-llama generic variants) from
_SERVER_NAMES so only llama-specific names remain (e.g., "llama-server",
"llama_server", "llama-server.exe"); update any dependent binary-discovery logic
that reads _SERVER_NAMES to rely on the narrowed list (no functional code
changes beyond the constant).
Comment on lines
+211
to
+222
| # Monkey-patch the module-level console used inside orchestrator | ||
| import opendrop.core.orchestrator as _orch_mod | ||
|
|
||
| original_console = _orch_mod.console | ||
| _orch_mod.console = _QueuingConsole() # type: ignore[assignment] | ||
| try: | ||
| orch.pull(source, token=token, quant_override=quant) | ||
| except Exception as exc: | ||
| asyncio.run_coroutine_threadsafe(queue.put(f"ERROR: {exc}"), loop) | ||
| finally: | ||
| _orch_mod.console = original_console | ||
| asyncio.run_coroutine_threadsafe(queue.put(None), loop) |
There was a problem hiding this comment.
Critical: Monkey-patching module-level console creates race condition.
Lines 211-222 mutate the module-level opendrop.core.orchestrator.console global. If multiple clients call /v1/pull concurrently, the threads will race to overwrite _orch_mod.console, causing output from one pull operation to leak into another client's stream.
Consider these alternatives:
- Preferred: Refactor
Orchestrator.pull()to accept an optionalconsoleparameter so each call gets its own isolated console instance. - Alternative: Use a thread-local console or a per-request console registry to avoid shared mutable state.
🔧 Example: Add console parameter to Orchestrator.pull
In opendrop/core/orchestrator.py, modify the pull method signature:
def pull(self, source: str, token: str | None = None, quant_override: str | None = None, console: Console | None = None) -> None:
_console = console or opendrop.core.orchestrator.console
# Use _console instead of the module-level console throughout the methodThen in the endpoint:
+ rich_console = Console(file=buf, highlight=False, markup=False)
orch = Orchestrator()
- # Monkey-patch the module-level console used inside orchestrator
- import opendrop.core.orchestrator as _orch_mod
-
- original_console = _orch_mod.console
- _orch_mod.console = _QueuingConsole() # type: ignore[assignment]
try:
- orch.pull(source, token=token, quant_override=quant)
+ orch.pull(source, token=token, quant_override=quant, console=_QueuingConsole())
except Exception as exc:
asyncio.run_coroutine_threadsafe(queue.put(f"ERROR: {exc}"), loop)
- finally:
- _orch_mod.console = original_console
- asyncio.run_coroutine_threadsafe(queue.put(None), loop)🤖 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 `@opendrop/inference/server.py` around lines 211 - 222, The code currently
monkey-patches the module-level opendrop.core.orchestrator.console which causes
races; refactor Orchestrator.pull to accept an optional console parameter (e.g.,
def pull(self, source, token=None, quant_override=None, console=None)) and
inside pull use a local _console = console or opendrop.core.orchestrator.console
everywhere instead of the module global; then update the server handler to
instantiate _QueuingConsole and pass it as the console argument to orch.pull
rather than assigning to _orch_mod.console and remove the try/finally
monkey-patch block so each request uses its own console instance and no shared
global is mutated.
Comment on lines
+224
to
+231
| async def _event_stream() -> AsyncIterator[str]: | ||
| loop.run_in_executor(None, _run_pull) | ||
| while True: | ||
| line = await queue.get() | ||
| if line is None: | ||
| yield "data: [DONE]\n\n" | ||
| break | ||
| yield f"data: {line}\n\n" |
There was a problem hiding this comment.
Resource leak: Background pull task not tracked or cancellable.
Line 225 calls loop.run_in_executor(None, _run_pull) without awaiting the returned Future. This means:
- The background task is not tracked, so if the client disconnects early (broken SSE connection), the pull operation continues running.
- There's no way to cancel the pull if the client aborts.
This wastes resources by running orchestrator operations that will never be consumed.
🛠️ Proposed fix: Track and cancel background task
async def _event_stream() -> AsyncIterator[str]:
- loop.run_in_executor(None, _run_pull)
+ task = loop.run_in_executor(None, _run_pull)
+ try:
- while True:
- line = await queue.get()
- if line is None:
- yield "data: [DONE]\n\n"
- break
- yield f"data: {line}\n\n"
+ while True:
+ line = await queue.get()
+ if line is None:
+ yield "data: [DONE]\n\n"
+ break
+ yield f"data: {line}\n\n"
+ finally:
+ # If client disconnects, attempt to cancel the background task
+ if not task.done():
+ task.cancel()Note: This won't immediately stop the synchronous _run_pull, but it will prevent further queue operations and signal intent to stop.
📝 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.
Suggested change
| async def _event_stream() -> AsyncIterator[str]: | |
| loop.run_in_executor(None, _run_pull) | |
| while True: | |
| line = await queue.get() | |
| if line is None: | |
| yield "data: [DONE]\n\n" | |
| break | |
| yield f"data: {line}\n\n" | |
| async def _event_stream() -> AsyncIterator[str]: | |
| task = loop.run_in_executor(None, _run_pull) | |
| try: | |
| while True: | |
| line = await queue.get() | |
| if line is None: | |
| yield "data: [DONE]\n\n" | |
| break | |
| yield f"data: {line}\n\n" | |
| finally: | |
| # If client disconnects, attempt to cancel the background task | |
| if not task.done(): | |
| task.cancel() |
🤖 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 `@opendrop/inference/server.py` around lines 224 - 231, Assign and track the
background executor Future returned by loop.run_in_executor when starting
_run_pull (e.g., store it in a variable like pull_future) and wrap the SSE loop
in try/finally so you can cancel and await that Future when the client
disconnects; specifically, in _event_stream obtain pull_future =
loop.run_in_executor(None, _run_pull), then in the finally block if not
pull_future.done() call pull_future.cancel() and await it (handling
asyncio.CancelledError/other exceptions), and also ensure you signal the
synchronous _run_pull to stop further queue.put()s (for example by putting None
or checking a shared flag) so no orphaned queue operations occur.
Comment on lines
+215
to
+216
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Log exceptions instead of silently swallowing them.
The bare except Exception: pass suppresses all errors during refresh, making debugging impossible when issues occur. At minimum, log the exception so operators can diagnose problems.
📋 Proposed fix to log exceptions
except Exception:
- pass
+ log: RichLog = self.query_one("#log-panel") # type: ignore[assignment]
+ log.write("[bold red]Error during auto-refresh[/bold red]")Or for more detailed logging:
except Exception as e:
- pass
+ log: RichLog = self.query_one("#log-panel") # type: ignore[assignment]
+ log.write(f"[bold red]Error during auto-refresh: {e}[/bold red]")🤖 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 `@opendrop/ui/tui.py` around lines 215 - 216, Replace the bare "except
Exception: pass" in opendrop/ui/tui.py with proper logging: catch the exception
as "except Exception as e" and call a logger (e.g., logger.exception(...) or
logging.exception(...)) to record the stack trace and context (import logging or
use the module/class logger already defined in this file); keep the swallow
behavior only if intentional—otherwise consider re-raising or handling specific
exceptions in the surrounding refresh/loop function that contains the current
except block.
Comment on lines
185
to
196
| async function loadHardware() { | ||
| // OpenDrop doesn't expose hardware via API yet; show placeholder | ||
| document.getElementById('hw-badge').textContent = 'OpenDrop local'; | ||
| try { | ||
| const r = await fetch(`${API}/v1/hardware`); | ||
| if (!r.ok) throw new Error('hardware fetch failed'); | ||
| const hw = await r.json(); | ||
| const gpuLabel = hw.gpu_name || hw.gpu_kind || 'CPU'; | ||
| const memGB = (hw.effective_memory_mb / 1024).toFixed(1); | ||
| document.getElementById('hw-badge').textContent = `${gpuLabel} · ${memGB} GB`; | ||
| } catch(e) { | ||
| document.getElementById('hw-badge').textContent = 'OpenDrop local'; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add defensive checks for hardware response fields.
Line 191 accesses hw.effective_memory_mb without verifying the field exists. If the API response structure changes or is incomplete, this will throw a JavaScript error and cause the fallback to "OpenDrop local" to trigger.
🛡️ Proposed fix: Guard field access
try {
const r = await fetch(`${API}/v1/hardware`);
if (!r.ok) throw new Error('hardware fetch failed');
const hw = await r.json();
const gpuLabel = hw.gpu_name || hw.gpu_kind || 'CPU';
- const memGB = (hw.effective_memory_mb / 1024).toFixed(1);
+ const memGB = hw.effective_memory_mb
+ ? (hw.effective_memory_mb / 1024).toFixed(1)
+ : '?';
document.getElementById('hw-badge').textContent = `${gpuLabel} · ${memGB} GB`;
} catch(e) {
document.getElementById('hw-badge').textContent = 'OpenDrop local';
}📝 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.
Suggested change
| async function loadHardware() { | |
| // OpenDrop doesn't expose hardware via API yet; show placeholder | |
| document.getElementById('hw-badge').textContent = 'OpenDrop local'; | |
| try { | |
| const r = await fetch(`${API}/v1/hardware`); | |
| if (!r.ok) throw new Error('hardware fetch failed'); | |
| const hw = await r.json(); | |
| const gpuLabel = hw.gpu_name || hw.gpu_kind || 'CPU'; | |
| const memGB = (hw.effective_memory_mb / 1024).toFixed(1); | |
| document.getElementById('hw-badge').textContent = `${gpuLabel} · ${memGB} GB`; | |
| } catch(e) { | |
| document.getElementById('hw-badge').textContent = 'OpenDrop local'; | |
| } | |
| } | |
| async function loadHardware() { | |
| try { | |
| const r = await fetch(`${API}/v1/hardware`); | |
| if (!r.ok) throw new Error('hardware fetch failed'); | |
| const hw = await r.json(); | |
| const gpuLabel = hw.gpu_name || hw.gpu_kind || 'CPU'; | |
| const memGB = hw.effective_memory_mb | |
| ? (hw.effective_memory_mb / 1024).toFixed(1) | |
| : '?'; | |
| document.getElementById('hw-badge').textContent = `${gpuLabel} · ${memGB} GB`; | |
| } catch(e) { | |
| document.getElementById('hw-badge').textContent = 'OpenDrop local'; | |
| } | |
| } |
🤖 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 `@opendrop/ui/web.py` around lines 185 - 196, In loadHardware, guard against
missing or malformed hw fields before using them: check that the fetched hw
object exists, that hw.effective_memory_mb is a finite number (fallback to 0 or
null) before dividing and calling toFixed, and that hw.gpu_name or hw.gpu_kind
are present before building gpuLabel; update the logic that computes memGB and
gpuLabel (and the assignment to document.getElementById('hw-badge').textContent)
to use safe defaults so missing fields do not throw and the catch only triggers
for network/errors.
Agent-Logs-Url: https://github.com/fernandogarzaaa/OpenDrop/sessions/2ad2fbdd-bbdf-4b54-b0cd-b54a36ee3793 Co-authored-by: fernandogarzaaa <99188227+fernandogarzaaa@users.noreply.github.com>
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
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
Quality-of-life and reliability upgrades for the 0.1.0b2 public beta — closes the remaining gaps we identified after PR #4 shipped the 0.1.0b1 cut.
API & Web UI
GET /v1/hardware— returns the hardware profile (OS, CPU, RAM, GPU, effective memory, backend priority) as JSONPOST /v1/pull— streams orchestrator output via SSE so the Web UI can show real pull/quantize progress instead of a blocking spinner[DONE]Cross-platform reliability
opendrop runhang/crash on Windows: replacedthreading.Event().wait()with a portablewhile True: time.sleep(1)+KeyboardInterruptloopLlamaCppServer.stop()usesTerminateProcesson Windows (noSIGTERM)llama-serverdiscovery now findsllama-server.exeand searches Windows install paths (Program Files\llama.cpp,~/llama.cpp/build/bin[/Release])require_server_binary()now emits platform-specific install hints (Windows release link, macOS Homebrew, Linux build instructions)CLI
opendrop list --search/-s <query>filters the registry by case-insensitive substring onidanddisplay_nameRegistry.search_models()supports the same filter at the data layer (matches acrossid,display_name,model_id,architecture,pipeline_tag)TUI
set_intervalkeeps the model table and running-state in sync without exiting and restarting the apprbinding now triggers a manual force-refresh instead of restartRelease plumbing
.github/workflows/publish.ymluses PyPI Trusted Publishing (OIDC) onv*.*.*andv*.*.*b*tagsCHANGELOG.mdbumped to[0.1.0b2] - 2026-05-14with all of the aboveTest plan
pytest tests/) — 119 passing (was 108, +11 new)ruff check opendrop/ tests/)ruff format --check)/v1/hardwareresponse shape,/v1/pull422 on missing source + SSE content-type,Registry.search_models(5 cases),opendrop list --searchfilteringtest_run_flowupdated to patchtime.sleepinstead ofthreading.Event.waitfor the new SIGTERM-safe loopopendrop serve, open Web UI, verify hardware badge populates and Pull button streams progressopendrop list --search llamaNotes
The PyPI publish workflow needs a trusted-publisher set up on PyPI for
opendrop(pypienvironment, this repo,publish.ymljob namepublish) before the first tag will succeed.Summary by CodeRabbit
Release Notes
New Features
listcommand now supports--searchflag to filter modelsBug Fixes
opendrop runerror on WindowsChores