Improve local development setup instructions in README #1079
Improve local development setup instructions in README #1079Khiladi-786 wants to merge 4 commits intofossasia:masterfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates README.md to add a concise, step-by-step local development setup guide (environment creation, dependency installation, running demo) plus a brief troubleshooting note for import issues, improving onboarding clarity for new contributors. Flow diagram for local development setup and troubleshootingflowchart TD
A(Clone_repository) --> B(Read_local_development_setup_section_in_README)
B --> C(Create_conda_env_visdom_env_python_3_10)
C --> D(Activate_conda_env_visdom_env)
D --> E(Pip_install_r_requirements_txt)
E --> F(Pip_install_pillow_and_charset_normalizer)
F --> G(Pip_install_editable_project_pip_install_e_dot)
G --> H(Run_demo_python_example_demo_py)
H --> I{Demo_runs_without_import_errors}
I -- Yes --> J(Access_application_at_http_localhost_8097)
I -- No --> K(Set_PYTHONPATH_pwd_py)
K --> H
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new local setup section uses
,,,bashinstead of Markdown code fences like ```bash, which will render incorrectly on GitHub and should be updated. - Consider separating the main setup commands from the troubleshooting tip (PYTHONPATH export) using a short explanatory sentence or a dedicated subsection so new contributors don’t confuse it as part of the default setup flow.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new local setup section uses `,,,bash` instead of Markdown code fences like ```bash, which will render incorrectly on GitHub and should be updated.
- Consider separating the main setup commands from the troubleshooting tip (PYTHONPATH export) using a short explanatory sentence or a dedicated subsection so new contributors don’t confuse it as part of the default setup flow.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 updates README.md to add “Local Development Setup (From Scratch)” instructions intended to help new contributors set up a fresh environment and run Visdom locally.
Changes:
- Added a new local development setup section with conda-based environment creation and install/run steps.
- Added a brief troubleshooting note for Python import/module path issues.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pip install pillow charset-normalizer | ||
| pip install -e . |
There was a problem hiding this comment.
pip install pillow charset-normalizer is likely unnecessary here: Pillow is already included in setup.py’s install_requires, so pip install -e . should pull it in. If there’s a known edge case (e.g., Pillow build failures on some platforms), it would be better documented as a troubleshooting note rather than a required step.
| ,,,bash | ||
| conda create -n visdom_env python=3.10 -y | ||
| conda activate visdom_env | ||
| pip install -r requirements.txt | ||
| pip install pillow charset-normalizer | ||
| pip install -e . | ||
| python example/demo.py | ||
|
|
||
| If you get module import errors: | ||
| export PYTHONPATH=$(pwd)/py | ||
| ,,,bash | ||
|
|
There was a problem hiding this comment.
The bash snippet isn’t fenced with valid Markdown code blocks. The ,,,bash lines won’t render as code, and there’s no closing fence around the commands. Please switch to standard Markdown fences (bash … ), and include the export PYTHONPATH=... line inside an appropriate code block (also remove the stray ,,,bash at the end).
| pip install -r requirements.txt | ||
| pip install pillow charset-normalizer |
There was a problem hiding this comment.
pip install -r requirements.txt will fail because there is no requirements.txt in the repository root. Consider removing this step or pointing to the actual dependency source (e.g., rely on pip install -e ., which already installs runtime requirements via setup.py).
| conda activate visdom_env | ||
| pip install -r requirements.txt | ||
| pip install pillow charset-normalizer | ||
| pip install -e . |
There was a problem hiding this comment.
The instructions run python example/demo.py but never instruct starting the Visdom server first. example/demo.py asserts a server connection (viz.check_connection(...)) and will fail unless visdom / python -m visdom.server is running (typically in another terminal). Please add an explicit “start server” step before running the demo.
| pip install -e . | |
| pip install -e . | |
| # In another terminal, start the Visdom server before running the demo: | |
| # visdom # or: python -m visdom.server |
README.md
Outdated
| If you get module import errors: | ||
| export PYTHONPATH=$(pwd)/py | ||
| ,,,bash |
There was a problem hiding this comment.
The troubleshooting note is currently ambiguous relative to the preceding pip install -e . step: with an editable install, import visdom should work without setting PYTHONPATH. If you keep this, clarify when it’s needed (e.g., only when running from source without installing) and consider showing it as an alternative command like PYTHONPATH=./py python -m visdom.server rather than a persistent shell export.
|
Thanks for the suggestion! I've updated the troubleshooting section to clarify when Please let me know if anything else should be improved. |
|
Thanks for the detailed review! I've updated the README to:
Please let me know if any further improvements are needed. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except OSError: | ||
| print(f"\nError: Port {port} is already in use.") | ||
| print("Try running on another port:") | ||
| print(f"python -m visdom.server -port {port+1}\n") | ||
| sys.exit(1) |
There was a problem hiding this comment.
The except OSError: handler assumes every OSError from app.listen(...) means "port already in use", but listen can fail for other reasons (e.g., permission denied, invalid address). Catch the exception as e, check e.errno (e.g., errno.EADDRINUSE / Windows equivalent), and re-raise or report a more accurate message for other error codes.
| ,,,bash | ||
| conda create -n visdom_env python=3.10 -y | ||
| conda activate visdom_env | ||
| pip install -r requirements.txt | ||
| pip install pillow charset-normalizer | ||
| pip install -e . | ||
| python example/demo.py |
There was a problem hiding this comment.
This section uses ,,,bash instead of a fenced code block (```bash). As written, the Markdown rendering will be broken and the commands may not display as code.
| pip install -r requirements.txt | ||
| pip install pillow charset-normalizer | ||
| pip install -e . |
There was a problem hiding this comment.
pip install -r requirements.txt will fail because this repository doesn't have a requirements.txt at the repo root. Either reference an existing requirements file or rely on pip install -e . (which installs install_requires from setup.py).
| If you are running directly from source without installing the package: | ||
|
|
||
| PYTHONPATH=./py python -m visdom.server | ||
| ,,,bash |
There was a problem hiding this comment.
The troubleshooting command is not inside a proper fenced code block (and the closing fence is also ,,,bash). Wrap PYTHONPATH=./py python -m visdom.server in a valid ```bash block so it renders correctly and is easy to copy/paste.
| try: | ||
| if bind_local: | ||
| app.listen(port, max_buffer_size=1024**3, address="127.0.0.1") | ||
| else: | ||
| app.listen(port, max_buffer_size=1024**3) | ||
|
|
||
| logging.info("Application Started") | ||
| logging.info(f"Working directory: {os.path.abspath(env_path)}") | ||
|
|
||
| except OSError: | ||
| print(f"\nError: Port {port} is already in use.") | ||
| print("Try running on another port:") | ||
| print(f"python -m visdom.server -port {port+1}\n") | ||
| sys.exit(1) |
There was a problem hiding this comment.
PR description focuses on README/local setup docs, but this change also alters server runtime behavior by handling OSError during app.listen(...) and exiting the process. Please confirm this behavior change is intended and update the PR description accordingly so reviewers/users understand the non-doc impact.
| import sys | ||
|
|
There was a problem hiding this comment.
sys is already imported at the module level (line 17). The additional import sys inside start_server is redundant and should be removed to avoid duplicate imports and keep imports consistent.
| import sys |
Description
This PR improves the local development setup instructions in the README to make it easier for new contributors to run the project from a fresh clone.
Added a clear step-by-step guide covering environment setup, dependency installation, and running the server and demo script. Also included a troubleshooting section for common import issues.
Motivation and Context
While setting up the project locally, I encountered issues related to:
visdominsidepy/)These changes aim to reduce setup friction and improve the onboarding experience for new contributors.
How Has This Been Tested?
Created a fresh conda environment (Python 3.10)
Installed dependencies using the updated instructions
Successfully ran:
python -m visdom.serverpython example/demo.pyVerified that the application runs correctly on
http://localhost:8097Screenshots (if appropriate):
N/A
Types of changes
Checklist:
py/visdom/VERSIONaccording to Semantic VersioningSummary by Sourcery
Document a simple from-scratch local development workflow for running the project after a fresh clone.
Documentation: