feat: add --download_only flag and visdom-download CLI for offline setups#960
feat: add --download_only flag and visdom-download CLI for offline setups#960pankajbaid567 wants to merge 1 commit intofossasia:masterfrom
Conversation
Reviewer's GuideAdds a first-class offline asset download mechanism for Visdom by introducing a --download_only flag, a dedicated visdom-download entry point, and wiring module/CLI entry points to invoke a new download-only helper without starting the Tornado server. Sequence diagram for new CLI and module download_only invocationssequenceDiagram
actor User
participant Shell
participant ConsoleScript_visdom as visdom_console_script
participant ConsoleScript_visdom_download as visdom_download_console_script
participant PyModule_main as visdom_server___main__
participant RunServer as visdom_server_run_server
%% visdom --download_only path
User->>Shell: visdom --download_only
Shell->>ConsoleScript_visdom: invoke entry_point visdom
ConsoleScript_visdom->>RunServer: call download_scripts_and_run()
RunServer->>RunServer: parse_args()
RunServer->>RunServer: FLAGS.download_only == True
RunServer->>RunServer: download_scripts()
RunServer->>User: print Downloaded all required scripts. Exiting.
RunServer-->>ConsoleScript_visdom: return
%% visdom-download path
User->>Shell: visdom-download
Shell->>ConsoleScript_visdom_download: invoke entry_point visdom-download
ConsoleScript_visdom_download->>RunServer: call download_only()
RunServer->>RunServer: download_scripts()
RunServer->>User: print Downloaded all required scripts. Exiting.
RunServer-->>ConsoleScript_visdom_download: return
%% python -m visdom.server --download_only path
User->>Shell: python -m visdom.server --download_only
Shell->>PyModule_main: run visdom.server.__main__
PyModule_main->>PyModule_main: detect --download_only in sys.argv
PyModule_main->>RunServer: import download_only
PyModule_main->>RunServer: call download_only()
RunServer->>RunServer: download_scripts()
RunServer->>User: print Downloaded all required scripts. Exiting.
RunServer-->>PyModule_main: return
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Using the existing
visdomconsole script with--download_onlywill currently calldownload_scripts()twice (viadownload_scripts_and_run()and then again inmain()), so consider either wiring the entry point tomain()directly or skipping the initial download when the flag is present. - The download-only behavior is duplicated between the
if FLAGS.download_onlybranch inmain()and the separatedownload_only()function; it would be cleaner to havemain()delegate todownload_only()to keep the logic in one place. - Instead of manually checking for "--download_only" in
sys.argvinside__main__.py, consider delegating torun_server.main()and relying on its argument parsing so that option handling stays centralized and consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using the existing `visdom` console script with `--download_only` will currently call `download_scripts()` twice (via `download_scripts_and_run()` and then again in `main()`), so consider either wiring the entry point to `main()` directly or skipping the initial download when the flag is present.
- The download-only behavior is duplicated between the `if FLAGS.download_only` branch in `main()` and the separate `download_only()` function; it would be cleaner to have `main()` delegate to `download_only()` to keep the logic in one place.
- Instead of manually checking for "--download_only" in `sys.argv` inside `__main__.py`, consider delegating to `run_server.main()` and relying on its argument parsing so that option handling stays centralized and consistent.
## Individual Comments
### Comment 1
<location> `py/visdom/server/run_server.py:155-158` </location>
<code_context>
+ )
FLAGS = parser.parse_args()
+ if FLAGS.download_only:
+ download_scripts()
+ print("Downloaded all required scripts. Exiting.")
+ return
+
# Process base_url
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid running `download_scripts()` twice when `--download_only` is used via the `visdom` console script.
Because the `visdom` entry point always calls `download_scripts_and_run()`, which unconditionally runs `download_scripts()` before `main()`, adding another `download_scripts()` in the `if FLAGS.download_only` block makes `visdom --download_only` download twice. In contrast, `python -m visdom.server --download_only` (which uses `download_only()`) only downloads once.
To fix this, ensure `download_scripts()` is called in exactly one place per invocation, e.g. by:
- Moving the call into `main()` and gating it on parsed flags, or
- Letting `download_scripts_and_run()` inspect the flags/argv and skip its initial call when `--download_only` is present.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR adds a dedicated mechanism to download Visdom's static assets (JavaScript, CSS, fonts) without starting the Tornado server, addressing a common pain point for users with air-gapped/offline systems. Previously, users had to manually modify run_server.py to achieve this. The change provides three ways to invoke the download-only mode: visdom --download_only, visdom-download, and python -m visdom.server --download_only.
Changes:
- Added
--download_onlyCLI flag to the existing visdom command - Created new
visdom-downloadconsole script entry point - Implemented module invocation support via
__main__.py
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| setup.py | Adds visdom-download as a new console script entry point |
| py/visdom/server/run_server.py | Adds --download_only argument parser flag and new download_only() function |
| py/visdom/server/main.py | Routes --download_only flag to appropriate function when module is invoked |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3c6c79e to
0f7d947
Compare
…tups Adds a dedicated mechanism to download all required static assets (JS, CSS, fonts) without starting the Tornado server. This addresses the workflow described in issue fossasia#942 where users on air-gapped machines need to pre-download assets on an online machine. Three ways to use: - visdom --download_only - visdom-download - python -m visdom.server --download_only Closes fossasia#942
0f7d947 to
9928d0d
Compare
|
A few things worth addressing:
|
Description
Adds a dedicated mechanism to download all required static assets (JS, CSS, fonts) without starting the Tornado server. Users on air-gapped/offline machines currently have to manually hack
run_server.py(comment outmain()) just to download dependencies on an online machine — this change makes that a first-class feature.Three ways to use:
visdom --download_only— new flag on the existing CLIvisdom-download— new dedicated console-script entry pointpython -m visdom.server --download_only— module invocationFiles changed:
py/visdom/server/run_server.py— added--download_onlyarg to the parser and a newdownload_only()functionsetup.py— addedvisdom-downloadconsole-script entry pointpy/visdom/server/__main__.py— routes--download_onlytodownload_only()when invoked as a moduleMotivation and Context
Fixes #942. Users with offline Linux servers need to download Visdom's JS/CSS/font dependencies on an online machine and then copy them over. The current workaround requires manually editing
run_server.pyto comment outmain(). This change provides a clean, supported way to do that.How Has This Been Tested?
ast.parse— no syntax errors.--download_onlyflag triggersdownload_scripts()and exits without starting the Tornado server (no "It's Alive!" message).visdomcommand still works as before (backward compatible).Types of changes
Checklist:
py/visdom/VERSIONaccording to Semantic VersioningSummary by Sourcery
Introduce a dedicated offline asset download mode for Visdom without starting the server.
New Features: