|
| 1 | +# Visdom Server Refactoring — Continuation Roadmap |
| 2 | + |
| 3 | +> **Reference:** [PR #675 - Server Refactor](https://github.com/fossasia/visdom/pull/675) by Jack Urbanek |
| 4 | +
|
| 5 | +## Context |
| 6 | + |
| 7 | +PR #675 completed the **structural split** of the monolithic `server.py` (~2,000 lines) into modular files under `py/visdom/server/` and `py/visdom/utils/`. That work is merged and done. |
| 8 | + |
| 9 | +However, the PR explicitly outlined follow-up work in TODO comments (`socket_handlers.py:41-48`, `web_handlers.py:54-59`) that was **never completed**. The maintainer wants this remaining work picked up. This plan covers everything that's left, prioritized by impact and risk. |
| 10 | + |
| 11 | +### Current Modular Structure (from PR #675) |
| 12 | + |
| 13 | +``` |
| 14 | +py/visdom/ |
| 15 | +├── server/ |
| 16 | +│ ├── __main__.py (Entry point) |
| 17 | +│ ├── app.py (Application orchestration) |
| 18 | +│ ├── build.py (Asset management) |
| 19 | +│ ├── defaults.py (Configuration) |
| 20 | +│ ├── run_server.py (Server startup) |
| 21 | +│ └── handlers/ |
| 22 | +│ ├── base_handlers.py (Base handler classes) |
| 23 | +│ ├── socket_handlers.py (WebSocket handlers) |
| 24 | +│ └── web_handlers.py (HTTP handlers) |
| 25 | +└── utils/ |
| 26 | + ├── server_utils.py (Server utilities) |
| 27 | + └── shared_utils.py (Shared utilities) |
| 28 | +``` |
| 29 | + |
| 30 | +--- |
| 31 | + |
| 32 | +## Phase 1: Bug Fixes & Resource Safety |
| 33 | +**Effort: Small | Risk: Low | PRs: 1 | Dependencies: None** |
| 34 | + |
| 35 | +These are actual bugs and resource leaks that can cause data loss or event loop stalls in production. |
| 36 | + |
| 37 | +| # | Issue | File | Lines | Fix | |
| 38 | +|---|-------|------|-------|-----| |
| 39 | +| 1a | Resource leak in `loadfile()` | `py/visdom/__init__.py` | 137-143 | File opened without context manager; if assert fires, handle leaks. Rewrite with `with open()` | |
| 40 | +| 1b | Bare `except:` clause | `py/visdom/__init__.py` | 2647 | Catches `SystemExit`/`KeyboardInterrupt`. Change to `except ImportError:` | |
| 41 | +| 1c | Thread lifecycle bug | `py/visdom/__init__.py` | 648-651 | `setup_polling()` thread missing `daemon=True` (compare with `setup_socket()` at 729-733 which has it). Prevents clean process exit | |
| 42 | +| 1d | Thread-unsafe global | `py/visdom/utils/shared_utils.py` | 20 | `_seen_warnings = set()` accessed from multiple threads without lock | |
| 43 | +| 1e | Broad exception swallowing | `py/visdom/server/handlers/base_handlers.py` | 35, 57 | Narrow `except Exception` to specific exception types | |
| 44 | + |
| 45 | +--- |
| 46 | + |
| 47 | +## Phase 2: Eliminate Code Duplication |
| 48 | +**Effort: Small-Medium | Risk: Low | PRs: 1 | Dependencies: None (parallel with Phase 1)** |
| 49 | + |
| 50 | +### 2a. Consolidate 13 duplicate `initialize()` methods |
| 51 | +The same 6-line `initialize(self, app)` is copy-pasted across 13 handlers in `web_handlers.py`: |
| 52 | +- `PostHandler` (line 61), `ExistsHandler` (line 90), `UpdateHandler` (line 115), `DeleteEnvHandler` (line 423), `EnvStateHandler` (line 471), `ForkEnvHandler` (line 491), `CompareHandler`, `DataHandler`, `SaveHandler`, etc. |
| 53 | + |
| 54 | +**Fix:** Move into `BaseHandler` in `base_handlers.py`. Handlers needing extras (`IndexHandler` needs `user_credential`, `wrap_socket`, `base_url`) call `super().initialize(app)` then add their own. |
| 55 | + |
| 56 | +### 2b. Deduplicate `delete_env` logic |
| 57 | +Identical environment deletion code exists in: |
| 58 | +- `web_handlers.py` `DeleteEnvHandler.wrap_func` (lines 432-460) |
| 59 | +- `socket_handlers.py` `on_message` delete_env branch (lines 127-155) |
| 60 | + |
| 61 | +**Fix:** Extract to `delete_env_files(env_path, eid)` in `server_utils.py`. |
| 62 | + |
| 63 | +### 2c. Deduplicate `get_rand_id` |
| 64 | +Two implementations: |
| 65 | +- `shared_utils.py:36` — `str(uuid.uuid4())` (good) |
| 66 | +- `__init__.py:107` — time-based hex (weak, not unique under rapid calls) |
| 67 | + |
| 68 | +**Fix:** Use UUID version everywhere. |
| 69 | + |
| 70 | +--- |
| 71 | + |
| 72 | +## Phase 3: Data Model Layer (Core Architecture Work) |
| 73 | +**Effort: Large | Risk: Medium | PRs: 2-3 | Dependencies: Phase 2** |
| 74 | + |
| 75 | +This is the primary TODO from PR #675: *"move the logic that actually parses environments and layouts to new classes in the data_model folder"* |
| 76 | + |
| 77 | +### 3a. Create `py/visdom/server/data_model/` package |
| 78 | + |
| 79 | +| File | Purpose | |
| 80 | +|------|---------| |
| 81 | +| `environment.py` | `Environment` class wrapping `{"jsons": {}, "reload": {}}` dict. Methods: `get_window()`, `set_window()`, `remove_window()`, `list_windows()`, `get_reload()` | |
| 82 | +| `state_manager.py` | `StateManager` class wrapping the top-level `state` dict. Methods: `get_env()`, `create_env()`, `delete_env()`, `list_envs()`, `fork_env()`, `serialize()`, `serialize_all()`. Absorbs functions from `server_utils.py` (`serialize_env`, `serialize_all`, `load_env`, `gather_envs`, `compare_envs`) | |
| 83 | +| `window.py` | Typed window structures (dataclasses/TypedDicts) replacing raw dicts built in `server_utils.py:window()` (lines 202-258) | |
| 84 | + |
| 85 | +### 3b. Refactor handlers to use data model |
| 86 | +- Replace `handler.state[eid]["jsons"][win]` patterns in `web_handlers.py` and `socket_handlers.py` with `state_manager.get_env(eid).get_window(win)` |
| 87 | +- Move `register_window()` and `window()` from `server_utils.py` into data model |
| 88 | +- Move `LazyEnvData` (server_utils.py:84-118) into `data_model/environment.py` |
| 89 | + |
| 90 | +### Target Structure After Phase 3 |
| 91 | + |
| 92 | +``` |
| 93 | +py/visdom/server/ |
| 94 | +├── data_model/ |
| 95 | +│ ├── __init__.py |
| 96 | +│ ├── environment.py (Environment class + LazyEnvData) |
| 97 | +│ ├── state_manager.py (StateManager - centralized state ops) |
| 98 | +│ └── window.py (Typed window dataclasses) |
| 99 | +├── handlers/ |
| 100 | +│ ├── base_handlers.py (Shared initialize logic) |
| 101 | +│ ├── socket_handlers.py (Uses StateManager, not raw dicts) |
| 102 | +│ └── web_handlers.py (Uses StateManager, not raw dicts) |
| 103 | +└── ... |
| 104 | +``` |
| 105 | + |
| 106 | +--- |
| 107 | + |
| 108 | +## Phase 4: Async I/O Modernization |
| 109 | +**Effort: Medium-Large | Risk: Medium | PRs: 2 | Dependencies: Phase 3** |
| 110 | + |
| 111 | +### 4a. Wrap blocking file I/O with `run_in_executor` |
| 112 | +Currently only **ONE** place uses async I/O (`socket_handlers.py:123` for `serialize_all`). All others block the Tornado event loop: |
| 113 | + |
| 114 | +| File | Lines | Blocking Operation | |
| 115 | +|------|-------|-------------------| |
| 116 | +| `server_utils.py` | 72 | Cookie file write | |
| 117 | +| `server_utils.py` | 121-155 | `serialize_env()` — JSON file write | |
| 118 | +| `server_utils.py` | 273-303 | `compare_envs()` — env file read | |
| 119 | +| `app.py` | 141 | Layout save (file write) | |
| 120 | +| `app.py` | 155 | Layout load (file read) | |
| 121 | +| `app.py` | 183 | State load (JSON file reads) | |
| 122 | +| `app.py` | 238, 242 | CSS file reads | |
| 123 | + |
| 124 | +With Phase 3's `StateManager`, all file I/O concentrates in one place — easy to wrap. |
| 125 | + |
| 126 | +### 4b. Convert handlers to `async def` / `await` |
| 127 | +Tornado 6+ supports native coroutines. Convert all handler `get()`/`post()`/`on_message()` to `async def`. Update `check_auth` decorator in `server_utils.py:49`. |
| 128 | + |
| 129 | +### 4c. Remove legacy compatibility shims |
| 130 | +- `collections.abc.Sequence` fallback in 3 files (`__init__.py:19-24`, `server_utils.py:30-35`, `web_handlers.py:26-31`) |
| 131 | +- Python 2 assert at `__init__.py:51` |
| 132 | +- PyTorch < 0.4 deprecation warnings (`__init__.py:407-410`) |
| 133 | +- Lua Torch deprecation notice (`web_handlers.py:76-81`) |
| 134 | + |
| 135 | +### 4d. Standardize string formatting |
| 136 | +Mixed `%`, `.format()`, and f-strings throughout. Standardize on f-strings. |
| 137 | + |
| 138 | +--- |
| 139 | + |
| 140 | +## Phase 5: Client Package Split (`__init__.py`) |
| 141 | +**Effort: Large | Risk: Medium | PRs: 3-4 | Dependencies: Phase 1 | Can parallel with Phases 3-4** |
| 142 | + |
| 143 | +The 2,712-line `__init__.py` monolith needs splitting: |
| 144 | + |
| 145 | +| PR | New File | Content | Lines Moved | |
| 146 | +|----|----------|---------|-------------| |
| 147 | +| 5a | `py/visdom/client/utils.py` | Utility functions: `get_rand_id`, `isstr`, `isnum`, `nan2none`, `loadfile`, `_axisformat`, `_opts2layout`, etc. | ~350 lines (106-450) | |
| 148 | +| 5b | `py/visdom/client/core.py` | `Visdom` class: init, session, connection, event handlers, save/fork/close | ~520 lines (452-975) | |
| 149 | +| 5c | `py/visdom/client/plotting.py` | All visualization methods: `text`, `image`, `scatter`, `line`, `heatmap`, `bar`, etc. | ~1,700 lines (977-2712) | |
| 150 | +| 5d | Update `__init__.py` | Re-exports only, preserving backward compatibility | imports only | |
| 151 | + |
| 152 | +--- |
| 153 | + |
| 154 | +## Phase 6: Socket Protocol Standardization |
| 155 | +**Effort: Medium | Risk: Low | PRs: 2 | Dependencies: Phases 3, 4** |
| 156 | + |
| 157 | +From TODO at `socket_handlers.py:47-48`: *"standardize the code between the client-server and visdom-server socket edges"* |
| 158 | + |
| 159 | +### 6a. Define message protocol |
| 160 | +Create `py/visdom/server/protocol.py` with typed message definitions (dataclasses): |
| 161 | +- **Server->Client:** `AliveMessage`, `WindowMessage`, `WindowUpdateMessage`, `LayoutUpdateMessage` |
| 162 | +- **Client->Server:** `CloseCommand`, `SaveCommand`, `DeleteEnvCommand`, `ForwardToVisCommand` |
| 163 | + |
| 164 | +### 6b. Unify polling and WebSocket message formatting |
| 165 | +The TODO at `socket_handlers.py:238` notes inconsistent message JSON formatting between paths. Ensure consistent encoding across all communication channels. |
| 166 | + |
| 167 | +--- |
| 168 | + |
| 169 | +## Open Issues Alignment |
| 170 | + |
| 171 | +| Issue | Related Phase | How Addressed | |
| 172 | +|-------|---------------|---------------| |
| 173 | +| [#989](https://github.com/fossasia/visdom/issues/989) (async race condition in env switching) | Phases 3 + 4 | StateManager with proper locking + async I/O | |
| 174 | +| [#1324](https://github.com/fossasia/visdom/issues/1324) (refactor image rendering logic) | Phases 3 + 5 | Typed window models + plotting extraction | |
| 175 | +| [#1310](https://github.com/fossasia/visdom/issues/1310) (multi-tenant architecture) | Phase 3 | StateManager provides isolation foundation | |
| 176 | + |
| 177 | +--- |
| 178 | + |
| 179 | +## TODO Debt Resolved (17+ items) |
| 180 | + |
| 181 | +| TODO Location | Phase | |
| 182 | +|---------------|-------| |
| 183 | +| `socket_handlers.py:41` — data model | Phase 3 | |
| 184 | +| `socket_handlers.py:43` — base handler init | Phase 2 | |
| 185 | +| `socket_handlers.py:45` — abstract app refs | Phase 3 | |
| 186 | +| `socket_handlers.py:47` — standardize protocol | Phase 6 | |
| 187 | +| `web_handlers.py:54-59` — same 4 TODOs | Phases 2, 3, 6 | |
| 188 | +| `web_handlers.py:147` — embeddings update flow | Phase 3 | |
| 189 | +| `web_handlers.py:166` — python client function | Phase 5 | |
| 190 | +| `web_handlers.py:478` — env state handler | Phase 3 | |
| 191 | +| `__init__.py:602` — merge setup_polling/setup_socket | Phase 5 | |
| 192 | +| `__init__.py:779` — investigate/deprecate send | Phase 5 | |
| 193 | +| `server_utils.py:56` — shared method call | Phase 2 | |
| 194 | +| `base_handlers.py:80` — error.html page | Phase 2 | |
| 195 | +| `socket_handlers.py:238` — message format | Phase 6 | |
| 196 | + |
| 197 | +--- |
| 198 | + |
| 199 | +## Execution Order & Parallelism |
| 200 | + |
| 201 | +``` |
| 202 | +Phase 1 (bugs) ──────────────────────────→ Phase 5 (client split) ──→ |
| 203 | +Phase 2 (dedup) ──→ Phase 3 (data model) ──→ Phase 4 (async) ──→ Phase 6 (protocol) |
| 204 | +``` |
| 205 | + |
| 206 | +Phases 1 & 2 can start immediately in parallel. Phase 5 can proceed independently after Phase 1. Phases 3 -> 4 -> 6 are sequential. |
| 207 | + |
| 208 | +**Estimated total: 10-13 PRs across all phases.** |
| 209 | + |
| 210 | +--- |
| 211 | + |
| 212 | +## Verification Plan |
| 213 | + |
| 214 | +For each phase: |
| 215 | +1. Run existing tests: `cd py && python -m pytest ../test/` |
| 216 | +2. Run Cypress E2E: `npm run test` (server on port 8098) |
| 217 | +3. Manual smoke test: `python -m visdom.server -port 8098` then run `example/demo.py` |
| 218 | +4. Verify no regressions in visual regression screenshots |
| 219 | +5. Run linting: `black py` and `npm run lint` |
| 220 | + |
| 221 | +--- |
| 222 | + |
| 223 | +## Critical Files Reference |
| 224 | + |
| 225 | +| File | Lines | Touched By | |
| 226 | +|------|-------|------------| |
| 227 | +| `py/visdom/__init__.py` | 2,712 | Phases 1, 5 | |
| 228 | +| `py/visdom/server/handlers/web_handlers.py` | 708 | Phases 2, 3, 4 | |
| 229 | +| `py/visdom/server/handlers/socket_handlers.py` | 409 | Phases 2, 3, 4, 6 | |
| 230 | +| `py/visdom/server/handlers/base_handlers.py` | 84 | Phase 2 | |
| 231 | +| `py/visdom/utils/server_utils.py` | 568 | Phases 2, 3, 4 | |
| 232 | +| `py/visdom/server/app.py` | 248 | Phases 3, 4 | |
| 233 | +| `py/visdom/utils/shared_utils.py` | 62 | Phase 1 | |
0 commit comments