-
Notifications
You must be signed in to change notification settings - Fork 0
Expand file tree
/
Copy pathsecond_opinion
More file actions
101 lines (77 loc) · 6.56 KB
/
Copy pathsecond_opinion
File metadata and controls
101 lines (77 loc) · 6.56 KB
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
# LemonTop Code Review & Optimization Proposals
This document outlines identified flaws, bugs, UI/UX issues, and performance optimization proposals for the **LemonTop** codebase.
---
## 1. 🐛 Bugs & Correctness Issues
### 1.1 Sparkline Block Character Typo
* **File:** [`src/lemontop/formatting.py:38`](file:///g:/My%20Drive/Projects/LemonTop/src/lemontop/formatting.py#L38)
* **Current Behavior:** The `blocks` string starts with a glitched quadrant character `▁` instead of the lower one-eighth block ` ` (`\u2581`):
```python
blocks = "▁▂▃▄▅▆▇█"
```
* **Impact:** The lowest non-zero data points in sparklines render a split quadrant symbol instead of a bottom bar graph line.
* **Proposed Fix:** Replace `blocks = "▁▂▃▄▅▆▇█"` with standard block elements: `blocks = " ▂▃▄▅▆▇█"`.
---
### 1.2 Sparkline Left-Padding Artifacts
* **File:** [`src/lemontop/formatting.py:49`](file:///g:/My%20Drive/Projects/LemonTop/src/lemontop/formatting.py#L49)
* **Current Behavior:** `sparkline()` uses `.rjust(width, "─")` to pad history lists shorter than `width`.
* **Impact:** When history buffers are short, empty values are rendered as horizontal border lines (`─`), making early telemetry graphs look like a solid high-value line across the left side of the chart.
* **Proposed Fix:** Change padding to spaces `"".join(output).rjust(width, " ")`.
---
### 1.3 Request History Flooded During Token Generation
* **File:** [`src/lemontop/collector.py:236-256`](file:///g:/My%20Drive/Projects/LemonTop/src/lemontop/collector.py#L236-L256)
* **Current Behavior:** `_last_request_key` includes `snapshot.output_tokens` and `snapshot.tokens_per_second`:
```python
key = (
snapshot.model,
snapshot.input_tokens,
snapshot.output_tokens,
snapshot.tokens_per_second,
snapshot.ttft_seconds,
)
```
* **Impact:** During active generation, `output_tokens` increments on every poll. Because the key changes every second, `LiveCollector` prepends a new `RequestRecord` on *every single refresh*. Within 8 seconds, the 8-item request table is filled with duplicate snapshots of the exact same in-flight request.
* **Proposed Fix:** Update the current active request record in `_history` while a request is in `"GENERATING"` state rather than prepending a new record on every token update.
---
### 1.4 Unchecked `journalctl` Subprocess Execution on Non-Linux Platforms
* **File:** [`src/lemontop/collector.py:313-337`](file:///g:/My%20Drive/Projects/LemonTop/src/lemontop/collector.py#L313-L337)
* **Current Behavior:** `_journal_lines()` attempts to execute `journalctl` via `subprocess.run()` on every refresh cycle without checking the underlying host OS.
* **Impact:** On Windows or macOS (or systems without `systemd`), it spawns a failing subprocess every second, appending `journalctl unavailable: [Errno 2] No such file or directory` errors to the UI log.
* **Proposed Fix:** Guard execution with `if platform.system() != "Linux": return []` or verify binary availability once during collector initialization.
---
## 2. 🖥️ UI & Keyboard Navigation Flaws
### 2.1 Cursor & Selection Reset in Request Table
* **File:** [`src/lemontop/app.py:242-256`](file:///g:/My%20Drive/Projects/LemonTop/src/lemontop/app.py#L242-L256)
* **Current Behavior:** `_update_requests()` calls `table.clear(columns=False)` on every 1.0s refresh.
* **Impact:** Calling `clear()` wipes all rows and resets the table cursor coordinate back to `(0, 0)`. If a user focuses the table (`t`) and uses keyboard navigation to select a request, their selection is wiped every second.
* **Proposed Fix:** Update table cells in-place or store and restore `table.cursor_coordinate` across updates.
---
### 2.2 Event Log Scrollback Wiped on Refresh
* **File:** [`src/lemontop/app.py:258-272`](file:///g:/My%20Drive/Projects/LemonTop/src/lemontop/app.py#L258-L272)
* **Current Behavior:** `_update_events()` calls `log.clear()` on every refresh.
* **Impact:** Completely clears log history every second, destroying the user's ability to scroll up (`l` key) and review older event/journal messages.
* **Proposed Fix:** Maintain an in-memory set or de-duplicated buffer of seen log entries and call `log.write()` incrementally for new lines without calling `log.clear()`.
---
### 2.3 Unused `Panel.panel_title` & Native Border Titles
* **File:** [`src/lemontop/app.py:20-26`](file:///g:/My%20Drive/Projects/LemonTop/src/lemontop/app.py#L20-L26)
* **Current Behavior:** `Panel` receives `title` in `__init__` and assigns `self.panel_title = title`, but `self.panel_title` is never rendered. Panel titles are instead printed inside the text body.
* **Impact:** Misses out on Textual's native `border_title` styling.
* **Proposed Fix:** Set `self.border_title = title` inside `Panel.__init__` to embed clean `btop`-style title text directly into the top panel border.
---
## 3. ⚡ Performance & Network Optimizations
### 3.1 Redundant 404 HTTP Probing Every Second
* **File:** [`src/lemontop/collector.py:67-92`](file:///g:/My%20Drive/Projects/LemonTop/src/lemontop/collector.py#L67-L92)
* **Current Behavior:** `LemonadeClient.get()` tries `/api/v1/{endpoint}` first, and falls back to `/v1/{endpoint}` upon receiving a `404`. It does not cache the successful prefix.
* **Impact:** For Lemonade servers configured with `/v1/`, LemonTop sends up to 4 unnecessary 404 HTTP requests per second (8 total requests per polling cycle).
* **Proposed Fix:** Store `self._cached_prefix` after the first successful HTTP request to skip failed prefix attempts on subsequent polls.
---
### 3.2 Sequential Blocking HTTP Requests
* **File:** [`src/lemontop/collector.py:113-137`](file:///g:/My%20Drive/Projects/LemonTop/src/lemontop/collector.py#L113-L137)
* **Current Behavior:** Telemetry requests (`health`, `stats`, `system-stats`, `system-info`) execute sequentially in a single thread.
* **Impact:** Total poll latency is additive (up to 3.2s on high-latency connections), causing `refresh_snapshot` to skip cycles or block the event loop thread pool.
* **Proposed Fix:** Use a `ThreadPoolExecutor` or `asyncio.gather` to fetch endpoints concurrently.
---
### 3.3 Multi-GPU / APU sysfs Monitoring Limitation
* **File:** [`src/lemontop/collector.py:277-311`](file:///g:/My%20Drive/Projects/LemonTop/src/lemontop/collector.py#L277-L311)
* **Current Behavior:** `_collect_amd_sysfs` breaks after inspecting the first device matching `/sys/class/drm/card*/device`.
* **Impact:** On systems with an AMD APU + discrete GPU or multiple GPUs, only `card0` is monitored.
* **Proposed Fix:** Add support for aggregating metrics across GPUs or specifying a target card index.