gate on min uv version and shortcut python candidate search if known#4489
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 enhances the installation process by ensuring compatibility with the 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: 6d30c50b11
ℹ️ 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".
| _uv_version_ok() { | ||
| _ver=$("$1" --version 2>/dev/null | awk '{print $2}') || return 1 | ||
| [ -n "$_ver" ] || return 1 | ||
| _oldest=$(printf '%s\n%s\n' "$UV_MIN_VERSION" "$_ver" | sort -V | head -n1) |
There was a problem hiding this comment.
The new _uv_version_ok helper relies on sort -V, which is a GNU extension and is not available in the default BSD sort on macOS. In that environment this check fails and the script always treats uv as too old, forcing an unnecessary reinstall attempt; this can make installs fail in offline or restricted environments even when a sufficiently new uv is already present.
Useful? React with 👍 / 👎.
![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 introduces two main changes: it enforces a minimum version for the uv package manager in install.sh, and it adds a shortcut in studio/setup.sh to use a pre-selected Python interpreter, avoiding a potentially slow search. The changes are logical and well-implemented. I have one suggestion to improve code clarity in install.sh.
| UV_MIN_VERSION="0.7.14" | ||
|
|
||
| _uv_version_ok() { | ||
| _ver=$("$1" --version 2>/dev/null | awk '{print $2}') || return 1 |
There was a problem hiding this comment.
The || return 1 at the end of this line is unlikely to ever be triggered. The exit code of a pipeline is that of its last command (awk here), which will succeed even with empty input (e.g., if the uv --version command fails). The next line [ -n "$_ver" ] || return 1 already correctly handles the case of an empty version string. Removing the redundant || return 1 would improve code clarity.
| _ver=$("$1" --version 2>/dev/null | awk '{print $2}') || return 1 | |
| _ver=$("$1" --version 2>/dev/null | awk '{print $2}') |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 885bf7312a
ℹ️ 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".
| if [ "$_a_part" -gt "$_b_part" ] 2>/dev/null; then | ||
| return 0 | ||
| fi | ||
| if [ "$_a_part" -lt "$_b_part" ] 2>/dev/null; then | ||
| return 1 |
There was a problem hiding this comment.
The new version_ge logic assumes each dot-separated token is purely numeric, so versions like 0.7.13-rc1 or 0.7.13+build make both integer comparisons fail here and the function eventually returns success. That causes _uv_version_ok to treat an older uv as meeting UV_MIN_VERSION, skipping the reinstall path and allowing the later uv pip install ... --torch-backend=auto step to fail on unsupported uv builds.
Useful? React with 👍 / 👎.
danielhanchen
left a comment
danielhanchen
left a comment
There was a problem hiding this comment.
Review of PR #4489
Thanks for the PR. The direction is good -- gating on a minimum uv version, making the llama.cpp build non-fatal, and shortcutting the Python search are all worthwhile. However there are a few issues that need fixing before merge.
Bug 1 (HIGH): TTY detection change breaks curl | sh install path
The autolaunch condition was changed from [ -t 0 ] to [ -t 1 ] && [ -t 2 ].
When installed via curl -fsSL url | sh:
- fd 0 (stdin) = pipe from curl, NOT a TTY
- fd 1 (stdout) = user terminal, IS a TTY
- fd 2 (stderr) = user terminal, IS a TTY
The original [ -t 0 ] correctly detected the piped install and skipped autolaunch. The new condition passes in the curl | sh case, so Studio auto-launches unexpectedly when the user merely ran the one-line install.
Suggested fix: Use [ -t 0 ] && [ -t 1 ] (both stdin and stdout must be TTYs) to correctly handle both curl | sh and ./install.sh > log.txt cases.
Also: printing "To launch, run: ..." instructions right before auto-launching is redundant and confusing -- the instructions should only appear in the non-interactive branch.
Bug 2 (MEDIUM): version_ge fails open on pre-release/suffixed versions
When a version segment contains non-numeric characters (e.g. 0.7.13-rc1, 0.7.14-dev), both [ -gt ] and [ -lt ] fail silently due to 2>/dev/null. The loop falls through without returning, and the function returns 0 (passes).
Concrete example: version_ge "0.7.13-rc1" "0.7.14" returns success, so a pre-release uv would incorrectly pass the minimum version gate.
Suggested fix: Strip pre-release suffixes in _uv_version_ok before passing to version_ge:
_uv_version_ok() {
_ver=$("$1" --version 2>/dev/null | awk '{print $2}') || return 1
[ -n "$_ver" ] || return 1
# Strip pre-release/build suffixes
case "$_ver" in
''|*[!0-9.]*) _ver=${_ver%%[-+]*} ;;
esac
case "$_ver" in
''|*[!0-9.]*) return 1 ;;
esac
version_ge "$_ver" "$UV_MIN_VERSION"
}Bug 3 (MEDIUM): REQUESTED_PYTHON_VERSION shortcut bypasses version range validation
The new shortcut path only checks if the binary is executable (-x) but skips the 3.11 <= minor <= 3.13 range check that the normal discovery loop enforces. If REQUESTED_PYTHON_VERSION points to an out-of-range interpreter, it gets accepted and the "compatible (3.11.x - 3.13.x)" message is printed incorrectly.
While install.sh currently always passes a 3.13 venv Python, this env var is a general mechanism and should enforce the same contract as the discovery loop.
Suggested fix: After accepting the executable, validate its version:
if [ -n "${REQUESTED_PYTHON_VERSION:-}" ] && [ -x "$REQUESTED_PYTHON_VERSION" ]; then
_req_ver=$("$REQUESTED_PYTHON_VERSION" --version 2>&1 | awk '{print $2}')
_req_minor=$(echo "$_req_ver" | cut -d. -f2)
if [ "$_req_minor" -ge "$MIN_PY_MINOR" ] 2>/dev/null && \
[ "$_req_minor" -le "$MAX_PY_MINOR" ] 2>/dev/null; then
BEST_PY="$REQUESTED_PYTHON_VERSION"
echo "Using requested Python version: $BEST_PY"
else
echo "Ignoring requested Python $REQUESTED_PYTHON_VERSION ($_req_ver) -- outside supported range"
fi
fiCode quality: run_quiet / run_quiet_no_exit duplication
The two functions are identical except for exit vs return. This duplicates ~14 lines. Consider consolidating into a single helper with a mode parameter:
_run_quiet() {
local on_fail="$1"; local label="$2"; shift 2
local tmplog; tmplog=$(mktemp)
if "$@" > "$tmplog" 2>&1; then rm -f "$tmplog"; return 0; fi
local exit_code=$?
echo "Failed: $label (exit code $exit_code):"; cat "$tmplog"; rm -f "$tmplog"
[ "$on_fail" = "exit" ] && exit "$exit_code" || return "$exit_code"
}
run_quiet() { _run_quiet exit "$@"; }
run_quiet_no_exit() { _run_quiet return "$@"; }That said, the fix itself (using return instead of exit so || BUILD_OK=false actually fires) is correct and a good catch -- the old run_quiet with exit made the || fallback dead code.
Minor: naming convention
version_ge lacks the underscore prefix used by other private helpers in install.sh (_is_pkg_installed, _smart_apt_install, _uv_version_ok). Should be _version_ge for consistency.
Minor: debug leftover
echo "finished finding best python" at line 251 of studio/setup.sh is a development leftover (pre-existing, not introduced by this PR, but worth cleaning up since this area is being touched).
|
@danielhanchen for the first one I was under the impression we wanted |
danielhanchen
left a comment
danielhanchen
left a comment
There was a problem hiding this comment.
Thank you for the PR! The goal of this PR is to harden the installer and Studio setup by gating on a minimum uv version, shortcutting Python discovery, making llama.cpp builds non-fatal, and adding auto-port selection. As a summary, this PR adds a version_ge() helper to enforce uv >= 0.7.14, introduces REQUESTED_PYTHON_VERSION to skip redundant Python discovery in studio/setup.sh, refactors run_quiet into _run_quiet/run_quiet_no_exit so llama.cpp build failures don't abort setup, and auto-selects a free port in 8888-8898 for Studio launch.
| Reviewers | Severity | Finding |
|---|---|---|
| 8/8 | High | REQUESTED_PYTHON_VERSION only validates minor version -- a hypothetical Python 4.x would pass |
| 8/8 | High | exit 1 on port exhaustion (8888-8898) fails an otherwise successful install |
| 7/8 | Medium | [ -t 1 ] && [ -t 2 ] (was [ -t 0 ]) now auto-launches under `curl |
| 8/8 | Medium | _run_quiet() drops local declarations, leaking on_fail/label/tmplog/exit_code into global scope |
Concrete suggestions for each finding below.
| _req_ver=$("$REQUESTED_PYTHON_VERSION" --version 2>&1 | awk '{print $2}') | ||
| _req_minor=$(echo "$_req_ver" | cut -d. -f2) | ||
| if [ "$_req_minor" -ge "$MIN_PY_MINOR" ] 2>/dev/null && \ | ||
| [ "$_req_minor" -le "$MAX_PY_MINOR" ] 2>/dev/null; then |
There was a problem hiding this comment.
[8/8 reviewers] High -- REQUESTED_PYTHON_VERSION only validates the minor version, not the major version. A hypothetical Python 4.12.x would pass the _req_minor range check and be accepted.
Extract _req_major and add a [ "$_req_major" -eq 3 ] guard:
| _req_ver=$("$REQUESTED_PYTHON_VERSION" --version 2>&1 | awk '{print $2}') | |
| _req_minor=$(echo "$_req_ver" | cut -d. -f2) | |
| if [ "$_req_minor" -ge "$MIN_PY_MINOR" ] 2>/dev/null && \ | |
| [ "$_req_minor" -le "$MAX_PY_MINOR" ] 2>/dev/null; then | |
| _req_ver=$("$REQUESTED_PYTHON_VERSION" --version 2>&1 | awk '{print $2}') | |
| _req_major=$(echo "$_req_ver" | cut -d. -f1) | |
| _req_minor=$(echo "$_req_ver" | cut -d. -f2) | |
| if [ "$_req_major" -eq 3 ] 2>/dev/null && \ | |
| [ "$_req_minor" -ge "$MIN_PY_MINOR" ] 2>/dev/null && \ | |
| [ "$_req_minor" -le "$MAX_PY_MINOR" ] 2>/dev/null; then |
| STUDIO_PORT=$(find_open_port "$START_PORT" "$END_PORT") || { | ||
| echo "Error: could not find an open port for Unsloth Studio between 8888 and 8898." | ||
| echo " To launch, free port 8888 and run:" | ||
| echo "" | ||
| echo " source ${VENV_NAME}/bin/activate" | ||
| echo " unsloth studio -H ${STUDIO_HOST} -p 8888" | ||
| echo "" | ||
| exit 1 | ||
| } | ||
| echo " To launch, run:" | ||
| echo "" | ||
| echo " source ${VENV_NAME}/bin/activate" | ||
| echo " unsloth studio -H ${STUDIO_HOST} -p ${STUDIO_PORT}" | ||
| echo "" | ||
| echo "==> Auto Launching Unsloth Studio..." | ||
| echo "" | ||
| exec "$VENV_NAME/bin/unsloth" studio -H 0.0.0.0 -p 8888 | ||
| exec "$VENV_NAME/bin/unsloth" studio -H "$STUDIO_HOST" -p "$STUDIO_PORT" |
There was a problem hiding this comment.
[8/8 reviewers] High -- exit 1 here causes the entire installer to report failure when no port in 8888-8898 is free, even though the install itself succeeded. CI pipelines and wrapper scripts will see a non-zero exit code and treat the run as broken.
Replace with a non-fatal fallback that prints manual instructions and skips auto-launch. Also adds </dev/tty to the exec so Studio gets a proper tty when invoked via curl | sh:
| STUDIO_PORT=$(find_open_port "$START_PORT" "$END_PORT") || { | |
| echo "Error: could not find an open port for Unsloth Studio between 8888 and 8898." | |
| echo " To launch, free port 8888 and run:" | |
| echo "" | |
| echo " source ${VENV_NAME}/bin/activate" | |
| echo " unsloth studio -H ${STUDIO_HOST} -p 8888" | |
| echo "" | |
| exit 1 | |
| } | |
| echo " To launch, run:" | |
| echo "" | |
| echo " source ${VENV_NAME}/bin/activate" | |
| echo " unsloth studio -H ${STUDIO_HOST} -p ${STUDIO_PORT}" | |
| echo "" | |
| echo "==> Auto Launching Unsloth Studio..." | |
| echo "" | |
| exec "$VENV_NAME/bin/unsloth" studio -H 0.0.0.0 -p 8888 | |
| exec "$VENV_NAME/bin/unsloth" studio -H "$STUDIO_HOST" -p "$STUDIO_PORT" | |
| STUDIO_PORT=$(find_open_port "$START_PORT" "$END_PORT") || STUDIO_PORT="" | |
| if [ -n "$STUDIO_PORT" ]; then | |
| echo " To launch, run:" | |
| echo "" | |
| echo " source ${VENV_NAME}/bin/activate" | |
| echo " unsloth studio -H ${STUDIO_HOST} -p ${STUDIO_PORT}" | |
| echo "" | |
| echo "==> Auto Launching Unsloth Studio..." | |
| echo "" | |
| exec "$VENV_NAME/bin/unsloth" studio -H "$STUDIO_HOST" -p "$STUDIO_PORT" </dev/tty | |
| else | |
| echo "" | |
| echo "Note: could not find an open port for Unsloth Studio between ${START_PORT} and ${END_PORT}." | |
| echo " To launch manually, free a port and run:" | |
| echo "" | |
| echo " source ${VENV_NAME}/bin/activate" | |
| echo " unsloth studio -H ${STUDIO_HOST} -p 8888" | |
| echo "" | |
| fi |
| # in non-interactive environments (Docker, CI, cloud-init) just print instructions. | ||
| if [ -t 0 ]; then | ||
| echo "==> Launching Unsloth Studio..." | ||
| if [ -t 1 ] && [ -t 2 ]; then |
There was a problem hiding this comment.
[7/8 reviewers] Medium -- The old guard was [ -t 0 ] (stdin is a tty), which prevented auto-launch under curl | sh since stdin is a pipe. The new guard [ -t 1 ] && [ -t 2 ] checks stdout/stderr but not stdin, so curl https://... | sh will now trigger auto-launch.
If this is intentional (so curl | sh users get Studio automatically), consider adding a brief inline comment so future readers don't "fix" it back to [ -t 0 ]:
| if [ -t 1 ] && [ -t 2 ]; then | |
| if [ -t 1 ] && [ -t 2 ]; then # stdout+stderr only; intentionally skips stdin for curl|sh |
| on_fail=$1 | ||
| label=$2 | ||
| shift 2 | ||
|
|
||
| tmplog=$(mktemp) || { | ||
| printf '%s\n' "Failed to create temporary file" >&2 | ||
| [ "$on_fail" = "exit" ] && exit 1 || return 1 | ||
| } | ||
|
|
||
| if "$@" >"$tmplog" 2>&1; then | ||
| rm -f "$tmplog" | ||
| return 0 | ||
| else | ||
| local exit_code=$? | ||
| echo "❌ $label failed (exit code $exit_code):" | ||
| cat "$tmplog" | ||
| exit_code=$? |
There was a problem hiding this comment.
[8/8 reviewers] Medium -- _run_quiet() declares on_fail, label, tmplog, and exit_code without local, leaking them into the caller's (and global) scope. The original run_quiet() used local for all of these. Restore local to prevent variable pollution:
| on_fail=$1 | |
| label=$2 | |
| shift 2 | |
| tmplog=$(mktemp) || { | |
| printf '%s\n' "Failed to create temporary file" >&2 | |
| [ "$on_fail" = "exit" ] && exit 1 || return 1 | |
| } | |
| if "$@" >"$tmplog" 2>&1; then | |
| rm -f "$tmplog" | |
| return 0 | |
| else | |
| local exit_code=$? | |
| echo "❌ $label failed (exit code $exit_code):" | |
| cat "$tmplog" | |
| exit_code=$? | |
| local on_fail=$1 | |
| local label=$2 | |
| shift 2 | |
| local tmplog | |
| tmplog=$(mktemp) || { | |
| printf '%s\n' "Failed to create temporary file" >&2 | |
| [ "$on_fail" = "exit" ] && exit 1 || return 1 | |
| } | |
| if "$@" >"$tmplog" 2>&1; then | |
| rm -f "$tmplog" | |
| return 0 | |
| else | |
| local exit_code=$? |
…y comment, restore local
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
- Remove find_open_port, TTY-gated autolaunch, and </dev/tty redirection from install.sh; just print launch instructions - Remove unused BEST_MAJOR variable from studio/setup.sh - Remove stray "finished finding best python" debug echo - Fix stale comment "below 3.12" to "below 3.11"
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4db3bef01f
ℹ️ 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".
|
|
||
| # If the caller (e.g. install.sh) already chose a Python, use it directly. | ||
| if [ -n "${REQUESTED_PYTHON_VERSION:-}" ] && [ -x "$REQUESTED_PYTHON_VERSION" ]; then | ||
| _req_ver=$("$REQUESTED_PYTHON_VERSION" --version 2>&1 | awk '{print $2}') |
There was a problem hiding this comment.
With set -euo pipefail, the command substitution $("$REQUESTED_PYTHON_VERSION" --version ...) will terminate the whole script if that executable exits non-zero. That means a bad-but-executable REQUESTED_PYTHON_VERSION (for example a wrapper script that fails at runtime) now aborts setup before the existing fallback search can run, even though other valid Python 3.11–3.13 interpreters may be available.
Useful? React with 👍 / 👎.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several improvements to the installation scripts. It adds a version check for the uv package manager to ensure a minimum required version is present, which is crucial for compatibility with features like --torch_backend. The Python discovery in setup.sh is now shortcut if a specific Python version is provided, speeding up the setup process. A significant bug fix has been made by refactoring the run_quiet helper function, which now correctly allows the GGUF build process to continue even if some steps fail, preventing the setup from exiting prematurely. The pull request also removes the automatic launching of Unsloth Studio in interactive terminals, which makes the installer's behavior more consistent and predictable. Additionally, a comment highlights a potential issue in install.sh regarding numeric comparisons that hide errors, suggesting to let the script fail loudly for easier debugging.
| if [ "$_a_part" -gt "$_b_part" ] 2>/dev/null; then | ||
| return 0 | ||
| fi | ||
| if [ "$_a_part" -lt "$_b_part" ] 2>/dev/null; then | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
The redirection to /dev/null in the numeric comparisons (-gt, -lt) hides potential errors if a version component is not an integer. While _uv_version_ok validates the version string, it's safer to let the script fail loudly if a non-numeric value ever reaches this point. This would make debugging easier if the validation logic changes in the future. Given set -e is active, an error here would correctly halt execution.
| if [ "$_a_part" -gt "$_b_part" ] 2>/dev/null; then | |
| return 0 | |
| fi | |
| if [ "$_a_part" -lt "$_b_part" ] 2>/dev/null; then | |
| return 1 | |
| fi | |
| if [ "$_a_part" -gt "$_b_part" ]; then | |
| return 0 | |
| fi | |
| if [ "$_a_part" -lt "$_b_part" ]; then | |
| return 1 | |
| fi |
Let non-numeric version parts surface errors on stderr instead of being silently swallowed.
…nslothai#4489) * gate on min uv version and shortcut python candidate search if known * fix sort -V cross compat issue, run_quiet early exit on llamacpp, autolaunch * update launch message * Fix PR comments * auto launch and find open port * remove dev install * Fix review findings: major-version guard, non-fatal port fallback, tty comment, restore local * Remove autolaunch, clean up dead state and debug noise - Remove find_open_port, TTY-gated autolaunch, and </dev/tty redirection from install.sh; just print launch instructions - Remove unused BEST_MAJOR variable from studio/setup.sh - Remove stray "finished finding best python" debug echo - Fix stale comment "below 3.12" to "below 3.11" * Reject prerelease uv at exact minimum version boundary * Remove 2>/dev/null from version_ge numeric comparisons Let non-numeric version parts surface errors on stderr instead of being silently swallowed. --------- Co-authored-by: Daniel Han <danielhanchen@gmail.com>
2 participants
Gate uv min version in addition to uv presence. torch_backend isn't an available option in older uv versions so install might allow the older uv to pass but not recognize the --torch_backend flag. Gating on min uv version triggers a uv download.