Skip to content

[staging CI] unslothai/unsloth#6858 - #343

Closed
danielhanchen wants to merge 18 commits into
mainfrom
pr-6858-ci
Closed

danielhanchen wants to merge 18 commits into
mainfrom
pr-6858-ci

Conversation

@danielhanchen

Copy link
Copy Markdown
Owner

Staging CI for unslothai#6858 (PR head merged with current main). Do not merge.

rodboev and others added 17 commits July 3, 2026 22:45

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces several significant improvements, including lazy loading of inference components, robust chat turn-end EOS token resolution to prevent infinite loops, a standalone Vulkan VRAM probe with dedicated integrated GPU headroom management, and support for distributed MLX training and inference on Apple Silicon. Additionally, it adds a malware security gate for RAG embedding models, automatic HTTP retries for failed Xet downloads, and thread activity tracking in the database. The review feedback highlights three key improvement opportunities: converting bindir to an absolute path in _vulkan_probe.py to prevent Windows DLL directory errors, wrapping the int(tid) conversion in chat_eos.py within a try-except block to avoid potential crashes, and using a with open(...) context manager in embeddings.py to prevent resource leaks.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

def main() -> int:
if len(sys.argv) < 2:
return 0
bindir = sys.argv[1]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Converting bindir to an absolute path using os.path.abspath is highly recommended. If a relative path is passed to os.add_dll_directory on Windows, it will raise a ValueError and crash the probe.

Suggested change
bindir = sys.argv[1]
bindir = os.path.abspath(sys.argv[1])

Comment on lines +84 to +89
try:
tid = id_tokenizer.convert_tokens_to_ids(marker)
except Exception:
tid = None
if tid is not None and tid != unk and int(tid) >= 0:
ids.add(int(tid))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The int(tid) conversion and subsequent check should be wrapped inside the try...except block. If convert_tokens_to_ids returns an unexpected type (such as a list or a non-numeric string), int(tid) will raise a TypeError or ValueError outside the try block, crashing the entire function.

Suggested change
try:
tid = id_tokenizer.convert_tokens_to_ids(marker)
except Exception:
tid = None
if tid is not None and tid != unk and int(tid) >= 0:
ids.add(int(tid))
try:
tid = id_tokenizer.convert_tokens_to_ids(marker)
if tid is not None and tid != unk and int(tid) >= 0:
ids.add(int(tid))
except Exception:
pass

local = hf_hub_download(name, "modules.json", token = token or None)
except EntryNotFoundError:
return ()
data = json.loads(open(local).read())

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using open(local).read() leaves the file descriptor open until garbage collection runs, which can cause resource leaks and file locking issues on Windows. Please use a with open(...) block to ensure the file is closed properly.

Suggested change
data = json.loads(open(local).read())
with open(local, 'r', encoding = 'utf-8') as f:
data = json.loads(f.read())

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces several enhancements and robustness fixes across the training and inference backends. Key changes include the addition of DeepSeek-V4 support with custom compute buffer estimations, the implementation of a local request admission control queue for llama-server, lazy loading of inference backends, and a security gate to block unsafe RAG embedding models. It also refactors tool-call parsing and stripping to align GGUF and safetensors paths, adds MLX distributed training and inference support, and improves Windows/WSL AMD GPU detection. Additionally, chat threads now track and sort by last activity, and unstructured seed upload blocks can be deleted in bulk. The reviewer's feedback suggests improving the readability of a dense PowerShell block in install.ps1 used to restore uv index environment variables.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread install.ps1
return [int]$LASTEXITCODE
} finally {
$ErrorActionPreference = $prevEap
if ($savedUvIndex) { foreach ($n in $savedUvIndex.Keys) { if ($null -ne $savedUvIndex[$n]) { Set-Item "Env:$n" $savedUvIndex[$n] } } }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This line is functionally correct, but its density makes it difficult to read and debug. For better maintainability, especially within a finally block where clarity is crucial for ensuring correct cleanup, consider expanding this into multiple lines.

            if ($savedUvIndex) {
                foreach ($n in $savedUvIndex.Keys) {
                    if ($null -ne $savedUvIndex[$n]) { Set-Item "Env:$n" $savedUvIndex[$n] }
                }
            }

@danielhanchen

Copy link
Copy Markdown
Owner Author

Staging run finished; closing. Staging PRs exist to run CI on a spare queue and are never merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants