Add Intel Arc GPU detection and XPU PyTorch install to Windows installer - #7706
Add Intel Arc GPU detection and XPU PyTorch install to Windows installer#7706legobele wants to merge 19 commits into
Conversation
The installer's GPU detection chain (NVIDIA -> AMD ROCm -> else) has no Intel Arc/SYCL/XPU branch, so Intel Arc GPUs fall into the "none (chat-only / GGUF)" branch and get CPU PyTorch despite PyTorch publishing XPU wheels at download.pytorch.org/whl/xpu. This adds: - WMI-based Intel GPU detection (Arc, Iris, UHD, HD Graphics) - Torch XPU availability check for migrated/upgraded environments - An XPU PyTorch install path with the whl/xpu index - CPU fallback with a pointer to the Intel oneAPI docs when XPU isn't available - Updated messaging from "NVIDIA or AMD ROCm" to include Intel Arc The XPU wheels ship their own oneAPI runtime (intel-sycl-rt et al.) so no Intel oneAPI Base Toolkit is required for GPU training. Tested on: Windows 11, Intel Arc 140V GPU (8GB), PyTorch 2.9.0+xpu Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
The XPU index selected during GPU detection was overwritten by Get-TorchIndexUrl before the install branch read it, so Intel hosts still got CPU PyTorch while being told XPU wheels were being installed. - Move the XPU reroute after Get-TorchIndexUrl, and let an explicit pin win - Detect via Get-CimInstance (Get-WmiObject is absent in PowerShell 7) - Match only Arc / Data Center GPU, so UHD / HD / Iris Xe are not promised XPU - Split Intel GPU present from XPU-capable so the CPU fallback hint works - Bound the XPU torch trio like every other index (bare names resolved torch 2.13.0 + torchaudio 2.11.0 and pulled unsloth back to an old release) - Clear the XPU state after a CPU fallback, mirroring the ROCm path - Teach the index family, GPU branch and torch flavor helpers about xpu
|
Thanks for this, Intel Arc support in the Windows installer is something we want. The direction is right, and I pushed a commit to your branch to fix a few things that stopped the feature from actually engaging. Details below so nothing is a surprise. The main issue
The visible result on an Arc box was the installer printing "Intel GPU detected" and "PyTorch XPU (SYCL) wheels will be installed from ...", then printing "No NVIDIA GPU detected." and installing CPU PyTorch. The XPU branch only ran if Simulating both revisions across 34 Windows scenarios, before and after your commit were identical on every path except the messaging. What the commit changes
VerificationArc A770 / B580 / 140V now select the XPU index and take the XPU install branch. Iris Xe, UHD 620 and HD 4000 correctly stay on CPU with the accurate hint. NVIDIA, AMD ROCm, CPU only and hybrid Intel plus NVIDIA scenarios are byte for byte identical to Two follow ups, happy to take these separately
Also worth rebasing on latest main when you get a chance, the branch predates #7692. |
install.ps1 now classifies an /xpu index leaf as family xpu / branch xpu, so mirror the same two cases in _tauri_torch_index_family and _tauri_gpu_branch. These feed the [TAURI:DIAG] line only, and a Linux user can already reach the xpu index via UNSLOTH_TORCH_INDEX_FAMILY, where it previously reported auto/unknown. Linux Intel auto-detection is not added here.
|
Checked whether this needs an What I changed (7d45c78)My previous commit taught This matters today even without Linux detection, because a Linux user can already reach the XPU index by hand: Before, that install reported These feed the What I did not add, and whyAuto-detecting an Intel GPU on Linux and rerouting to the XPU index is a real gap, but it is a feature rather than a fix for this PR:
Since neither of us can test on an Arc box under Linux in this PR, I would rather land the Windows path here and do Linux detection separately where it can be exercised properly.
|
|
Ran a regression sweep across every vendor we support, since this touches the shared index and diagnostics helpers. No regressions found. Staging CI, including macOSReplicated onto a staging repo rather than loading the org queue. All green at Four runs show as cancelled at 10:41:29. Those were superseded three seconds later by the 10:41:32 runs on the same branch, which all passed, so it is concurrency group cancellation rather than failure. Every workflow has a green terminal run. Staging PRs closed, not merged. Vendor spoofing in an isolated venvBuilt a clean
Also spoofed each vendor in its own venv to check installer side leaf classification, identical on both branches: Real NVIDIA4x B200, driver 590.48.01, torch 2.9.1+cu128: Installer matricesThe tests that actually exercise the change: 34 Windows scenarios and 12 Linux/WSL/macOS scenarios, run against |
Comment and whitespace only, no code change.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- Run the Intel scan before the GPU report chain instead of inside its final else. A WMI-named-only AMD adapter set ROCmGpuLabel and took that chain, so a discrete Arc card next to an AMD CPU's integrated Radeon was never detected. The scan is gated on no usable NVIDIA or AMD, and the Intel branch ranks above the two AMD-present-but-unusable branches, so a usable AMD host is unaffected. - Let a migrated env's torch veto the hardware match only when it is itself an XPU build. A CPU build reports torch.xpu.is_available() False for lacking XPU support, not for unsuitable hardware, and was blocking the CPU to XPU upgrade. - Detect Intel in studio/setup.ps1 too. It only knew NVIDIA and AMD, so every successful Intel install printed none (chat-only / GGUF) right after install.ps1 reported a usable Arc GPU. Self-contained so studio update works.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- Reset $script:IsIntelXpu at the start of each invocation. Under the documented irm | iex path $script: is the caller's session scope, so a second run in the same session inherited a stale true, skipped the scan on a now-NVIDIA host and still rerouted to the xpu index. Reproduced in pwsh before fixing. - Gate the Intel scan on whether AMD actually gets a wheel, not on whether an AMD arch was seen. An arch missing from the family map has no ROCm wheels and lands on CPU torch, so it must not outrank a usable Arc card. The map is hoisted above the scan and consumed unchanged by the AMD reroute. - Select the XPU index in studio/setup.ps1, not just report it. Previously setup printed Intel GPU detected and then installed CPU torch, so studio update never migrated an Arc box off CPU. Adds a bounded XPU install with a CPU fallback, teaches the stale-venv check about +xpu, and mirrors the wheel-aware AMD gate so the two files agree instead of wiping the venv on every update.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- Force the dependency pass on an Arc host whose torch is not XPU-capable, the Intel counterpart of the existing AMD escape. Without it the fast up-to-date path skipped the install block, so the xpu index selection was never reached and a CPU venv never migrated. - Confirm a working XPU runtime before treating an xpu venv as stale. If CIM is unavailable or returns an Intel name outside the Arc match, the expected tag fell through to cpu and a valid XPU environment was rebuilt and lost. - Force-reinstall the XPU trio only when the installed wheel is not already +xpu, or the pin changed. It was unconditional, so a fresh install re-fetched multiple GB immediately and again on every update. - Warn when torch.xpu.is_available() is false after installing XPU torch, naming the Intel driver floor. Otherwise the installer promised GPU training while unsloth raised NotImplementedError at import on a stale driver. - Stop the detection probe vetoing the hardware match. Its cpu fallback could not displace the installed +xpu wheel, so it only mislabelled a capable GPU as unusable; the driver warning covers that case honestly, and setup.ps1 agrees.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…dbytes on the Intel path install.sh: teach _torch_flavor_tag, _expected_torch_flavor_tag and _torch_index_repairable about the xpu leaf. The diagnostic already reported gpu_branch=xpu, but an xpu pin fell to the custom arm so a migrated env kept its CPU wheel. The +xpu flavor arm is required alongside, otherwise a correct 2.10.0+xpu wheel reads as cpu and gets force-reinstalled every run. install.ps1 / studio/setup.ps1: route every torch probe through a new bounded Invoke-BoundedPythonProbe (ProcessStartInfo, both streams drained async, WaitForExit, kill on timeout). A hanging Intel driver init is exactly what these probes detect, and an unbounded one would hang the installer instead of reaching the warning. Timeouts read as not-available. Get-InstalledTorchTag now shares the helper rather than carrying a second copy of the pattern. install.ps1: install bitsandbytes>=0.50.0 on the XPU path. unsloth's floor is >=0.45.5, so a migrated venv keeps a pre-0.49 wheel with no XPU library and 4-bit QLoRA silently turns off. Same floor the AMD paths use, since <=0.49.2 NaNs at 4-bit decode and an Arc card can sit next to a Radeon.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
… pin studio/setup.ps1: `unsloth studio update` migrating a CPU venv to XPU replaced only the torch trio. install_python_stack.py then upgrades unsloth and unsloth-zoo alone, so an installed bitsandbytes 0.45.x kept satisfying the base floor while carrying no Windows XPU kernels, and 4-bit QLoRA silently turned off. Adds the same bitsandbytes>=0.50.0 --no-deps pass install.ps1 got, placed after the stack so it is the last word, gated on $XpuIndexUrl (the CPU fallback clears it, no-torch never sets it) and still inside the -not $SkipPythonDeps block so the up-to-date escape does not reach it. install.ps1: key the bitsandbytes pass off the index leaf instead of $script:IsIntelXpu. An explicit UNSLOTH_TORCH_INDEX_FAMILY=xpu pin on a non-Intel host skips the XPU branch but still installs the trio from the xpu index, so torch is +xpu and needs the same floor. The CPU fallback rewrites $TorchIndexUrl, so a failed XPU install reads as cpu and stays quiet.
Comment-only pass over the previous commit's additions, which had not been through one: 15 lines removed across the two bounded-scan headers, the two registry-fallback headers and the Triton block. Kept the facts that cost measurement: -OperationTimeoutSec not being enforced for a local COM session, Ok being false on an empty answer because a Windows host always has an adapter, the registry class key being fallback rather than fast path here, the 151 shared Triton paths, and why the uninstall has to be paired with a reinstall after the stack. No code tokens changed; verified with a PowerShell token-stream diff of both files, which also confirms the two helper copies stay identical.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…t strand the venv The replacement uninstalled triton-windows and then installed the XPU triton from the index. A failure between the two left the venv with a partially deleted triton, since the uninstall drops the paths shared with the XPU distribution, and the warning made that look like a skipped optional repair. The uninstall cannot go last, because it removes the paths in triton-windows' own record and those are the shared ones. So fetch first: pip download the wheel, confirm one is actually on disk (exit 0 alone is not enough, an sdist-only mirror satisfies that), and only then uninstall and install the local file. A local wheel installs with the network refused, so nothing after the destructive step depends on the index. A failed fetch leaves triton-windows in place, which is the pre-existing shadowing rather than a broken venv, and says so. Past that point only disk or permissions can fail, so restore triton-windows if the local install does, leaving a triton that imports. If both fail the message is loud and carries the repair command, with the index URL redacted since a mirror pin can carry a token. pip only: uv has no pip download (astral-sh/uv#3163).
|
@codex review |
1 similar comment
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…ll state up front Get-IntelRegistryAdapterNames wrapped the whole enumeration in a single try, so one unreadable subkey discarded every adapter found before it. windows_intel_gpu_in_registry(), the in-process Python probe over the same class key, skips per subkey and continues; the PowerShell copy now does too. It also matched on the PCI vendor id but returned DriverDesc, which the callers re-filter on "Intel", so a localized or OEM-branded Arc was found here and dropped there. Both installers carry the same copy and a test asserts they stay identical. setup.ps1 read $installedTorchTag and $XpuIndexUrl from outside the blocks that assign them. Unset and $null are both falsy so behaviour is unchanged, but a caller running with Set-StrictMode -Version Latest turned those reads into terminating errors, and install.ps1 is documented as irm | iex into the caller's own session. Two comment corrections: 0.48.2, not 0.49.0, is the first win_amd64 bitsandbytes wheel carrying libbitsandbytes_xpu.dll, and the triton package overlap is version-dependent rather than a fixed 151 paths. The new test drives the shipped helper with the registry cmdlets mocked rather than reading a hive, so it runs on Linux and macOS as well as Windows.
hardware.py has always emitted versions["xpu"], but HardwareInfo only ever declared cuda and rocm. On an Arc host both of those are null, so the runtime row disappeared entirely while the GPU name and VRAM rows still rendered, leaving a host that looks half detected. That was unreachable on Windows until the installer learned to select XPU wheels, which is what makes it worth fixing here. The three-way choice is lifted into a helper at module scope: inlining it pushes AboutTab past the cognitive-complexity ceiling. The label is a proper noun, so every locale carries the same literal.
|
Pushed two commits after simulating this end to end rather than only reasoning about it. Three of the findings are in code I added earlier in this PR. Registry fallback ( Two defects, both found by driving the shipped function with the registry cmdlets mocked:
22 scenarios, 3 cells changed, the other 19 (NVIDIA, AMD, CPU, stale entry, empty key, non-numeric subkey, class key unreadable) bit-identical. Negative control against the previous commit fails exactly those 3.
StrictMode (
Worth being precise about scope: Two comment corrections
About tab
What the simulations covered
One thing I checked and did not change: the comment on |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 237cf1e8f4
ℹ️ 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 ($script:IsIntelXpu -and $SkipPythonDeps) { | ||
| $_torchIsXpu = Test-TorchXpuAvailable -PythonExe "python" | ||
| if (-not $_torchIsXpu) { | ||
| substep "Intel GPU detected but PyTorch XPU is unavailable -- reinstalling XPU PyTorch" "Cyan" | ||
| $SkipPythonDeps = $false |
There was a problem hiding this comment.
When upgrading an existing Windows XPU environment whose torch.xpu.is_available() is already true, this check leaves $SkipPythonDeps enabled, so the newly added bitsandbytes upgrade and triton-windows replacement below never run. This affects migrations in particular: the first studio update executes the old installed setup.ps1, upgrades Unsloth, and cannot perform these new repairs; subsequent updates execute this version but take the fast path because Unsloth is current and XPU is available, leaving the old bitsandbytes wheel and triton-windows shadowing the XPU Triton indefinitely. The fast-path decision needs to detect those stale XPU dependencies or use a migration marker.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and the migration framing is the right one: the escape only fired when XPU was unavailable, so a venv already on +xpu took the fast path forever and never picked up either remediation. Fixed in 2603fc8 by probing for exactly what the two passes install for, bitsandbytes below 0.50.0 or triton-windows still present, and clearing the fast path when either is outstanding. An unreadable version reads as stale, which costs one dependency pass rather than leaving a venv that cannot do 4-bit.
New matrix rows: stale bnb -> pass forced; triton-windows present -> pass forced; unreadable bnb -> pass forced; deps current -> fast path kept; NVIDIA host with the same stale bnb -> fast path kept, the probe never runs off an Intel host.
| $_gpuScan = Invoke-BoundedVideoControllerScan | ||
| $_gpuNames = if ($_gpuScan.Ok) { $_gpuScan.Names } else { @(Get-IntelRegistryAdapterNames) } | ||
| $intelGpus = @($_gpuNames | Where-Object { $_ -match "(?i)Intel" }) |
There was a problem hiding this comment.
When Win32_VideoController succeeds but returns a localized Intel name without the ASCII word Intel (for example the Japanese Arc name already modeled in tests/studio/test_intel_registry_fallback.ps1), _gpuScan.Ok selects the WMI names exclusively and the following Intel filter drops the adapter. The registry helper was specifically written to normalize these localized names using VEN_8086, but it is only called when the entire WMI scan fails, so affected Arc hosts are classified as CPU-only. Retry or merge the registry results when the successful WMI result contains no recognizable Intel adapter.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, and it makes the helper I just fixed unreachable on the path that matters. Fixed in 2603fc8, but narrower than "retry or merge": the registry now re-labels an adapter WMI already reported, matched by name. A registry entry naming nothing in the WMI list stays ignored, so a driver record outliving its card still cannot promote a host WMI answered for, which is an existing control (R-Registry Arc, WMI answers).
Truth table from the matrix, all new rows: localized Arc + registry naming the same adapter -> xpu; localized Arc + unrelated stale registry Arc -> cpu; localized Arc + empty registry -> cpu; localized iGPU + matching registry -> Intel detected, not xpu. NVIDIA and AMD controls with a stale registry Arc unchanged.
| Fast-Uninstall "triton-windows" | Out-Null | ||
| if ($script:UnslothVerbose) { | ||
| Fast-Install --force-reinstall --no-deps $_tritonWheel | ForEach-Object { Redact-InstallOutput "$_" } | Out-Host |
There was a problem hiding this comment.
If setup is interrupted after Fast-Uninstall "triton-windows" but before the XPU wheel is reinstalled, the environment is left without the shared Triton files even though install_python_stack.py has already written the completion manifest immediately before returning. The next studio update can therefore accept that stale manifest and take the fast path, since the boot-dependency check only examines studio.txt and does not validate either Triton distribution. Remove the manifest before this destructive post-stack operation and write it only after all XPU remediation completes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. The round-9 pre-fetch shrank the window to uninstall-then-install but did not close it, and the manifest is written before either. Fixed in 2603fc8: the manifest is read and removed before the uninstall and the same bytes are written back after, gated on a triton actually being importable again. If neither the XPU wheel nor triton-windows reinstalls the manifest stays gone, which is what makes the next update repair instead of fast-pathing past a broken torch.compile.
The path is asked for via install_manifest.manifest_path() rather than assembled, so it cannot drift, and an older tree without the helper skips the hold entirely.
| substep "installing PyTorch from $(Remove-IndexUrlCredentials $TorchIndexUrl)..." | ||
| # Bound the trio like every other index: the xpu index serves torch past our | ||
| # ceiling (up to 2.13.0), and torchaudio dropped its exact torch pin. | ||
| $torchInstallExit = Invoke-InstallCommandRetry -Label "install PyTorch (Intel XPU)" { uv pip install --python $VenvPython --force-reinstall "torch>=2.4,<2.11.0" "torchvision>=0.19,<0.26.0" "torchaudio>=2.4,<2.11.0" --default-index $TorchIndexUrl } |
There was a problem hiding this comment.
The XPU install accepts torch 2.4 and 2.5 even though unsloth/models/_utils.py unconditionally raises RuntimeError for an XPU device when torch is below 2.6. This breaks installs against an XPU mirror that only contains an older wheel, and it also lets setup retain an existing 2.5+xpu wheel because that version satisfies the range. Use an XPU-specific lower bound of 2.6 so the installer cannot report success with a runtime that Unsloth immediately rejects.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed at unsloth/models/_utils.py:1929, which raises unconditionally for an XPU device below 2.6. Fixed in 2603fc8: torch>=2.6,<2.11.0 with torchvision>=0.21 and torchaudio>=2.6 on the XPU install in both installers and on the xpu arm of the flavor repair. The CPU fallback after a failed XPU install keeps 2.4, since that path is CPU torch.
Matrix diff: 53 rows moved on install.ps1 and 75 on setup.ps1, every one only on the install command, and every one on the xpu index. Nothing on a cu*, rocm or cpu index moved.
There was a problem hiding this comment.
Measuring the exposure after the fact, since I claimed more than I had checked: the official xpu index carries no torch below 2.6 at all. Earliest wheel on download.pytorch.org/whl/xpu is 2.6.0+xpu, for both cp3xx-win_amd64 and cp3xx-linux_x86_64, so the old 2.4 floor was never reachable there and torch>=2.4,<2.11.0 resolved to the same wheel the new range does. Confirmed across cp310 to cp313 on both platforms: 2.10.0+xpu either way.
So the change is defensive rather than a live break. It bites on a custom UNSLOTH_PYTORCH_MIRROR or UNSLOTH_TORCH_INDEX_URL carrying older wheels, and it makes the constraint state the floor Unsloth actually supports instead of one that happens to be safe because of what upstream publishes. The retain-an-existing-2.5+xpu case needs a wheel this index never shipped, so it would have to come from elsewhere.
Keeping the fix on those grounds, but the severity is lower than the report reads.
…and an old wheel Four holes in the XPU paths, all found by driving the shipped code rather than reading it. The registry fallback only ran when the CIM scan failed. When it succeeds and returns a localized adapter name, which on non-English Windows carries no ASCII "Intel", the filter dropped the adapter and the host went to CPU torch. The registry now re-labels an adapter WMI already reported, matched by name so an entry naming nothing WMI listed stays ignored: a driver record outliving its card still cannot promote a host WMI answered for. The XPU trio accepted torch 2.4 and 2.5, which unsloth/models/_utils.py rejects at import for an XPU device. An xpu mirror carrying only an older wheel produced an install that reported success and then failed on the first import, and an existing 2.5+xpu venv was kept because it satisfied the range. The floor is 2.6 on the XPU paths only; the CPU fallback keeps 2.4. The "package is up to date" fast path escaped for an Arc host on CPU torch, but not for one already on XPU torch whose bitsandbytes predates the XPU kernels or whose triton-windows still shadows the XPU Triton. Those two live in the dependency pass, so a venv that reached +xpu without them, an explicit pin or an update whose first pass ran the pre-XPU setup.ps1, never got them on any later update either. An unreadable version reads as stale. install_python_stack.py writes its completion manifest immediately before returning, so an interrupt between the triton-windows uninstall and the XPU wheel install left a venv with no triton that the next update read as complete. The manifest is now held aside across the swap and restored only once a triton is importable again.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2603fc809a
ℹ️ 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 ($_manifestSaved -and $_tritonPresent) { | ||
| try { Set-Content -LiteralPath $_manifestPath -Value $_manifestSaved -NoNewline -ErrorAction Stop } catch {} |
There was a problem hiding this comment.
On Windows PowerShell 5.1, Set-Content defaults to the system ANSI encoding when recreating this deleted file. For users whose venv path contains non-ASCII characters, this can corrupt the UTF-8 JSON that studio/install_manifest.py::read_manifest explicitly reads with encoding="utf-8"; every subsequent update then treats the installation as incomplete, reruns the dependency pass, and repeats the Triton replacement. Restore the original bytes or specify UTF-8 explicitly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, and worse than stated in one direction while the suggested route is unavailable in the other. Get-Content is ANSI on a BOM-less file in 5.1 too, so the read already lost bytes before the write got a chance to. And 5.1 has no BOM-less UTF-8: -Encoding utf8 there always emits a BOM, utf8NoBOM arrived in PowerShell 6, and read_manifest is json.load with encoding utf-8, which rejects a BOM outright ("Unexpected UTF-8 BOM"). I checked both failure modes locally: BOM raises JSONDecodeError, cp1252 bytes raise UnicodeDecodeError.
So in af928dd the manifest is MOVED into the wheel temp directory and moved back, with no encoding involved at either end. That directory is already deleted in the finally, which is what keeps an unrestored manifest gone.
Sources: about_Character_Encoding and Set-Content.
| $_manifestSaved = Get-Content -Raw -LiteralPath $_manifestPath -ErrorAction Stop | ||
| Remove-Item -LiteralPath $_manifestPath -Force -ErrorAction Stop | ||
| } catch { $_manifestSaved = $null } |
There was a problem hiding this comment.
If the manifest is briefly locked or cannot be removed, this catch only clears $_manifestSaved and then proceeds to uninstall triton-windows. The current fix therefore still leaves the old valid manifest in place during the destructive window; fresh evidence is that the removal failure at this exact catch is ignored, unlike the earlier dependency-pass removal which aborts. If the process is interrupted after the uninstall, the next update can accept the stale manifest and fast-path past the missing Triton files, so the swap should be skipped or aborted unless the manifest is confirmed absent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, that catch defeated the hold in exactly the case it exists for. Fixed in af928dd: a manifest that will not move now skips the swap outright rather than proceeding into the uninstall with the old valid one still in place. triton-windows keeps shadowing the XPU Triton, which costs torch.compile on the GPU and the next run can still fix, and that beats risking a venv with no Triton that reads as complete.
243 setup scenarios, 0 rows moved by the rework once the random temp directory names are normalised, and StrictMode still clean.
There was a problem hiding this comment.
Following up on my own fix here, because it was still wrong.
Modelling the manifest in the setup.ps1 scenario matrix (which had no coverage for it) showed the swap still running with the manifest in place: Move-Item across volumes is a copy followed by a delete, and it reports success when only the delete fails, leaving the original exactly where it was. Reproduced on pwsh 7 with a read-only parent directory: the call reports success, the destination exists, and so does the source. The abort added in af928dd therefore never fired, and a valid manifest stood through the whole destructive window, which is the case it exists to prevent.
83d188c confirms the move by testing the source path afterwards and treats a manifest still standing as a failure to move it, aborting the swap.
Four scenarios now cover it: swap succeeds and the manifest comes back byte for byte; neither Triton reinstalls and it stays gone; the fetch fails and it is never touched; it cannot move and the swap does not start. Before the fix the last one reported the swap running; after it, it does not. 247 scenarios, StrictMode still clean.
…rewriting it Two problems with the hold added in 2603fc8, both on the restore side. Reading and rewriting the file cannot survive a manifest carrying a non-ASCII path. Windows PowerShell 5.1 writes Set-Content in the ANSI code page by default, and its -Encoding utf8 emits a BOM that install_manifest.read_manifest's json.load rejects outright ("Unexpected UTF-8 BOM"); Get-Content is ANSI on a BOM-less file too, so the read lost bytes before the write got a chance to. The manifest is now MOVED into the wheel's temp directory and moved back, so no encoding is involved at either end. That directory is already removed in the finally, which is what keeps an unrestored manifest gone. A manifest that would not move left the old valid one in place for the whole destructive window, since the failure only cleared the saved copy and carried on into the uninstall. That is the case the hold exists for, so it now skips the swap entirely and says so: triton-windows keeps shadowing the XPU Triton, which costs torch.compile on the GPU and is repairable on the next run, rather than risking a venv with no Triton that reads as complete.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af928dd888
ℹ️ 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".
| # torch forever. $SkipPythonDeps is re-tested so an escape already taken above (AMD, | ||
| # anyio, incomplete install) does not probe twice. The probe is bounded, and a timeout | ||
| # reads as "not XPU" -- that costs one dependency pass, never a silent CPU venv. | ||
| if ($script:IsIntelXpu -and $SkipPythonDeps) { |
There was a problem hiding this comment.
On a mixed NVIDIA+Intel host using an explicit xpu index pin, $HasNvidiaSmi prevents both the hardware scan and the runtime promotion from setting $script:IsIntelXpu, even though the pin later authoritatively selects XPU. Consequently, an up-to-date migrated +xpu environment with old bitsandbytes or triton-windows still takes the fast path here and never reaches either remediation. Fresh evidence beyond the earlier fast-path report is this explicit-pin mixed-GPU path, which remains excluded by the $script:IsIntelXpu gate; the check should also recognize the pinned/installed XPU flavor.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same gating mistake as round 4, reintroduced in the check I added in 2603fc8. Fixed in bb71199.
The bitsandbytes pass had this exact shape before: gated on $script:IsIntelXpu, which a FAMILY=xpu pin on a non-scanned host never sets, and the fix then was to key off the index leaf instead. The leaf is not resolved yet at the fast path, but the installed flavour tag is, so the staleness check now fires when the scan says Intel OR the venv is already on a +xpu wheel. Whatever put it there, the two remediations apply. The runtime probe above stays on the scan, since reinstalling XPU torch is only right where an Intel GPU was actually found.
Two new matrix rows for the mixed NVIDIA + Intel box under UNSLOTH_TORCH_INDEX_FAMILY=xpu with a +xpu venv: stale bitsandbytes now forces the dependency pass with IsIntelXpu false, and current dependencies keep the fast path. A pure NVIDIA host on a cu wheel with the same stale bitsandbytes never runs the probe at all, which the matrix asserts as the control. 247 pre-existing scenarios, 0 moved.
Worth noting the harness needed widening first: its fast-path region anchor matched only the original if, so the new statement sat outside the extracted region and would never have run. The two new rows would have passed for nothing.
…n swap Move-Item across volumes is a copy followed by a delete, and it reports success when only the delete fails, leaving the original exactly where it was. So the guard added in af928dd could believe it had set the manifest aside while a valid one sat there for the whole destructive window, which is the case that guard exists to prevent. Found by modelling the manifest in the setup.ps1 scenario matrix, which this had no coverage for: with the parent directory read-only the swap still ran, and the locked scenario passed for the wrong reason. The move is now confirmed by testing the source path afterwards, and a manifest still standing aborts the swap like any other failure to move it. Four new scenarios cover it: the swap keeping a byte-identical manifest, a swap where neither Triton reinstalls correctly leaving it gone, a failed fetch never touching it, and a manifest that cannot move aborting the swap.
…ot just the GPU scan $HasNvidiaSmi suppresses the Intel scan, so on a mixed NVIDIA + Intel box under an explicit xpu pin $script:IsIntelXpu stays false while the pin still lands the venv on a +xpu wheel. The staleness check added in 2603fc8 was gated on that flag alone, so those hosts kept taking the fast path and never reached the bitsandbytes floor or the Triton replacement. This is the same gating mistake the bitsandbytes pass had in round 4, where the fix was to key off the index leaf rather than the scan. The leaf is not resolved yet at the fast path, but the installed flavor tag is, and whatever put the venv on a +xpu wheel the two remediations still apply. The runtime probe above stays on the scan: reinstalling XPU torch is only right where an Intel GPU was actually found. A pure NVIDIA host on a cu wheel never runs the probe, which the matrix asserts alongside the two new mixed-host rows.
|
@codex review |
2 participants
Fixes the installer's GPU detection chain which currently only checks for NVIDIA (CUDA) and AMD (ROCm), causing Intel Arc GPUs to fall into the cpu-only Torch path.
What changed
Why it works
PyTorch publishes XPU (SYCL) wheels at download.pytorch.org/whl/xpu that bundle their own oneAPI runtime (intel-sycl-rt, intel-cmplr-lib-rt, etc.), so no Intel oneAPI Base Toolkit installation is required for GPU training.
Tested on
Windows 11, Intel Arc 140V GPU (8GB), PyTorch 2.9.0+xpu, Qwen3.5-0.8B loaded on xpu:0
Related