Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience during Unsloth Studio setup by standardizing and improving the terminal output. It introduces a consistent, step-based visual style for setup logs across different operating systems, making the process clearer and easier to follow. A new verbose mode provides detailed diagnostic information for advanced users, while making certain build steps optional prevents complete setup failures due to non-critical component issues. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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: b4e6ba32d0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the setup experience by unifying the terminal output style across setup.sh, setup.ps1, and Python installation scripts. The new step-based format is much cleaner and easier to follow. The addition of a --verbose flag for detailed troubleshooting logs is a great feature. Making the llama.cpp build optional is also a smart move to improve user experience on systems with tricky toolchains.
I've found one issue in setup.ps1 where it exits on llama.cpp build failure, which is inconsistent with setup.sh and the PR's goal of making this step optional. My review includes a suggestion to fix this.
|
reverted this back to fail-fast behaviour in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a691e4117
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Resolve merge conflicts in setup.sh and setup.ps1 while preserving both main's behavioral additions and the PR's styling refactor: setup.sh: - Keep main's _run_quiet/run_quiet/run_quiet_no_exit architecture with PR's step() styled error messages - Restore REQUESTED_PYTHON_VERSION support from main (install.sh contract) with substep() output style - Restore graceful run_quiet_no_exit || BUILD_OK=false for llama.cpp clone/cmake/build steps (instead of hard exit) - Keep PR's try_quiet for optional llama-quantize step - Keep PR's compact python version check one-liners setup.ps1: - Restore main's cpu-first/CUDA-else PyTorch install structure with PR's step/substep/verbose styling - Combine main's conda+URL detail in Python-not-found error with PR's step/substep format - Style main's stale-venv and conda-skip messages to match PR conventions Auto-merged from main (no conflicts): - _platform_compat import and UTF-8 reconfigure in run.py - Test-IsConda, py.exe launcher, Get-Command -All Python discovery - Stale-venv rebuild logic - Get-PytorchCudaTag with cu118/cpu fallbacks
|
Merged main into this branch to resolve conflicts in What was conflicting and how it was resolved:
Auto-merged without conflicts (from main):
All syntax checks pass ( |
- Bracket IPv6 display_host in external_url to produce clickable URLs - Redirect try_quiet failure log to stderr instead of stdout - Clamp _step label to column width to prevent negative padding
for more information, see https://pre-commit.ci
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ed5b69c22
ℹ️ 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".
| ipv6_bind = bind_host in ("::", "::1") | ||
| if ipv6_bind: | ||
| local_url = f"http://[::1]:{port}" | ||
| alt_local = f"http://localhost:{port}" | ||
| else: | ||
| local_url = f"http://127.0.0.1:{port}" | ||
| alt_local = f"http://localhost:{port}" |
There was a problem hiding this comment.
Treating only ::/::1 as IPv6 means a bind_host of localhost always emits 127.0.0.1 URLs. On hosts where localhost resolves to IPv6 first (or IPv6-only loopback), Uvicorn can listen on ::1 while the printed local/API links point to IPv4 and fail to connect. This is a regression from using localhost-based URLs for loopback binds and affects users who start Studio with -H localhost.
Useful? React with 👍 / 👎.
Simulation harness (tests/simulate_pr4494.py) creates an isolated uv venv, copies the real source files into it, and runs subprocess tests for all three fixes with visual before/after demos and edge cases. Standalone bash test (tests/test_try_quiet.sh) validates try_quiet stderr redirect across 8 scenarios including broken-version contrast. 39 integration tests total (14 IPv6 + 15 try_quiet + 10 _step), all existing 75 unit tests still pass.
for more information, see https://pre-commit.ci
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
The %-15s printf format pads short labels but does not truncate long ones. Change to %-15.15s so labels wider than 15 chars are clipped, matching the PowerShell .Substring(0,15) and Python label[:15] logic.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83cab0c395
ℹ️ 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".
| external_url = f"http://{display_host}:{port}" | ||
| listen_all = bind_host in ("0.0.0.0", "::") | ||
| loopback_bind = bind_host in ("127.0.0.1", "localhost", "::1") | ||
| api_base = local_url if listen_all or loopback_bind else external_url |
There was a problem hiding this comment.
When bind_host is 0.0.0.0/::, api_base is forced to local_url, so the banner advertises 127.0.0.1/::1 API endpoints even though the server is intentionally exposed on another host address. In remote/server setups, users copying the shown /api URL from the banner get a non-routable loopback address instead of the reachable network address, which is a regression from the prior display_host-based API links.
Useful? React with 👍 / 👎.
These test files are not part of the styling fix and should not ship with this PR.
Resolve conflicts in install_python_stack.py: keep the PR's column- layout output system (_step, _COL, _LABEL) and incorporate main's _safe_print Unicode fallback for Windows consoles.
- install_python_stack.py: restore _red for patch_package_file warnings (was downgraded to _dim) - setup.ps1: capture winget output and show on failure for CUDA, Node, Python, and OpenSSL installs (was piped to Out-Null) - setup.ps1: always show git pull failure warning, not just in verbose mode
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly refactors the terminal output for Unsloth Studio's setup and startup processes across various platforms (Linux/macOS, Windows, Colab). It introduces a standardized, color-coded logging system using step and substep functions for consistent and user-friendly feedback during installation and runtime. This includes centralizing the startup banner generation into a new startup_banner.py module and updating install_python_stack.py and the setup.sh/setup.ps1 scripts to use these new output helpers. The unsloth studio setup CLI command now supports a --verbose flag for detailed logging. Additionally, the setup scripts' logic for detecting and installing dependencies like Node.js, Python, CUDA Toolkit, and llama.cpp build has been refined for improved robustness and clarity. The stdout_supports_color function in studio/backend/startup_banner.py should be updated to include Windows Virtual Terminal processing for better consistency and color support on Windows, and its broad except Exception: block should be modified to log the exception, adhering to the repository's rule against silent exception handling.
| def stdout_supports_color() -> bool: | ||
| """True if we should emit ANSI colors.""" | ||
| if os.environ.get("NO_COLOR", "").strip(): | ||
| return False | ||
| if os.environ.get("FORCE_COLOR", "").strip(): | ||
| return True | ||
| try: | ||
| return sys.stdout.isatty() | ||
| except Exception: | ||
| return False | ||
|
|
||
|
|
||
| def print_port_in_use_notice(original_port: int, new_port: int) -> None: | ||
| """Message when the requested port is taken and another is chosen.""" | ||
| msg = f"Port {original_port} is in use, using port {new_port} instead." | ||
| if stdout_supports_color(): | ||
| print(f"\033[38;5;245m{msg}\033[0m") | ||
| else: | ||
| print(msg) | ||
|
|
||
|
|
||
| def print_studio_access_banner( | ||
| *, | ||
| port: int, | ||
| bind_host: str, | ||
| display_host: str, | ||
| ) -> None: | ||
| """Pretty-print URLs after the server is listening (beginner-friendly).""" |
There was a problem hiding this comment.
For consistency with install_python_stack.py and to ensure colors work correctly on Windows, this function should also attempt to enable virtual terminal (VT) processing. The current implementation only checks if stdout is a TTY, which is not sufficient on its own for modern Windows terminals. Additionally, the broad except Exception: blocks have been modified to log the exception, adhering to the repository rule against silent exception handling for better debugging.
def stdout_supports_color() -> bool:
"""True if we should emit ANSI colors."""
if os.environ.get("NO_COLOR", "").strip():
return False
if os.environ.get("FORCE_COLOR", "").strip():
return True
try:
if not sys.stdout.isatty():
return False
except Exception as e:
# Log the exception for debugging purposes as per repository rule.
import logging
logging.debug(f"stdout.isatty() check failed: {e}")
return False
if sys.platform == "win32":
try:
import ctypes
kernel32 = ctypes.windll.kernel32
handle = kernel32.GetStdHandle(-11) # STD_OUTPUT_HANDLE
mode = ctypes.c_ulong()
kernel32.GetConsoleMode(handle, ctypes.byref(mode))
kernel32.SetConsoleMode(handle, mode.value | 0x0004)
except Exception as e:
# Log the exception for debugging purposes as per repository rule.
import logging
logging.debug(f"Failed to enable VT processing on Windows: {e}")
return False
return TrueReferences
- Avoid using broad, silent exception handlers like
except Exception: pass. Instead, log the exception, even if at a debug level, to aid in future debugging.
Same capture-and-print-on-failure pattern already used for Node, Python, CUDA, and OpenSSL winget installs.
2 participants
Unify Studio terminal output styling across setup flows with a cleaner, step-based format and shared banner treatment.
Update Windows setup (
studio/setup.ps1) to match the new visual style while keeping behavior intact.Add
unsloth studio setup --verbose(-v) to enable full dependency/build logs for troubleshooting.Windows:
WSL:
Launch:
