From cc4e1074c9afe84c6b32dbb03f2cc6c919993504 Mon Sep 17 00:00:00 2001 From: Deepratna Awale Date: Fri, 6 Nov 2026 10:51:34 +0000 Subject: [PATCH] Fix crash when searching in the Python Packages dialog Selecting a package loads its details (pip show / pip index) in a fire-and-forget task with no exception handler, so any failure other than a ProcessException was rethrown on the UI thread and took down the app. Typing in the search box moves the selection on almost every keystroke, starting one of these loads per keystroke, overlapping against the same venv. - Log failures of the details load instead of rethrowing them - Load one package's details at a time and skip loads whose package is no longer selected - Don't start a second load for an item that is already loading - PipIndexResult.Parse returns no versions instead of throwing when the output has no "Available versions:" line - PipShowResult.Parse no longer throws when "License:" has no following "Location:" line Fixes #693 --- .../Dialogs/PythonPackagesItemViewModel.cs | 14 +- .../Dialogs/PythonPackagesViewModel.cs | 109 +++++++++--- StabilityMatrix.Core/Python/PipIndexResult.cs | 18 +- StabilityMatrix.Core/Python/PipShowResult.cs | 9 +- .../Core/PipIndexResultTests.cs | 35 ++++ .../Core/PipShowResultsTests.cs | 36 ++++ .../PythonPackagesDialogTests.cs | 160 ++++++++++++++++++ 7 files changed, 337 insertions(+), 44 deletions(-) create mode 100644 StabilityMatrix.Tests/Core/PipIndexResultTests.cs create mode 100644 StabilityMatrix.UITests/PythonPackagesDialogTests.cs diff --git a/StabilityMatrix.Avalonia/ViewModels/Dialogs/PythonPackagesItemViewModel.cs b/StabilityMatrix.Avalonia/ViewModels/Dialogs/PythonPackagesItemViewModel.cs index 378eb1657..a3783e00f 100644 --- a/StabilityMatrix.Avalonia/ViewModels/Dialogs/PythonPackagesItemViewModel.cs +++ b/StabilityMatrix.Avalonia/ViewModels/Dialogs/PythonPackagesItemViewModel.cs @@ -4,13 +4,11 @@ using StabilityMatrix.Avalonia.ViewModels.Base; using StabilityMatrix.Core.Exceptions; using StabilityMatrix.Core.Helper; -using StabilityMatrix.Core.Models.FileInterfaces; using StabilityMatrix.Core.Python; -using StabilityMatrix.Core.Services; namespace StabilityMatrix.Avalonia.ViewModels.Dialogs; -public partial class PythonPackagesItemViewModel(ISettingsManager settingsManager) : ViewModelBase +public partial class PythonPackagesItemViewModel : ViewModelBase { [ObservableProperty] private PipPackageInfo package; @@ -81,9 +79,9 @@ value is null /// /// Loads the pip show result if not already loaded /// - public async Task LoadExtraInfo(DirectoryPath venvPath, PyBaseInstall pyBaseInstall) + public async Task LoadExtraInfo(Func> createVenvRunner) { - if (PipShowResult is not null) + if (PipShowResult is not null || IsLoading) { return; } @@ -98,11 +96,7 @@ public async Task LoadExtraInfo(DirectoryPath venvPath, PyBaseInstall pyBaseInst } else { - await using var venvRunner = await pyBaseInstall.CreateVenvRunnerAsync( - venvPath, - workingDirectory: venvPath.Parent, - environmentVariables: settingsManager.Settings.EnvironmentVariables - ); + await using var venvRunner = await createVenvRunner(); PipShowResult = await venvRunner.PipShow(Package.Name); diff --git a/StabilityMatrix.Avalonia/ViewModels/Dialogs/PythonPackagesViewModel.cs b/StabilityMatrix.Avalonia/ViewModels/Dialogs/PythonPackagesViewModel.cs index 3d19f6a2a..ae907b518 100644 --- a/StabilityMatrix.Avalonia/ViewModels/Dialogs/PythonPackagesViewModel.cs +++ b/StabilityMatrix.Avalonia/ViewModels/Dialogs/PythonPackagesViewModel.cs @@ -44,6 +44,20 @@ public partial class PythonPackagesViewModel : ContentDialogViewModelBase private readonly IPrerequisiteHelper prerequisiteHelper; private PyBaseInstall? pyBaseInstall; + /// + /// Lets only one package's details load at a time. Typing in the search box moves the + /// selection on almost every keystroke, which would otherwise start overlapping pip + /// processes against the same venv (each one rewriting its pyvenv.cfg). + /// + private readonly SemaphoreSlim extraInfoLock = new(1, 1); + + private CancellationTokenSource? extraInfoCts; + + /// + /// Replaces how venv runners are created, so tests can run without a real Python install. + /// + internal Func>? VenvRunnerFactory { get; set; } + public DirectoryPath? VenvPath { get; set; } public PyVersion? PythonVersion { get; set; } @@ -85,7 +99,7 @@ private void PostConstruct() .Connect() .DeferUntilLoaded() .Filter(searchPredicate) - .Transform(p => new PythonPackagesItemViewModel(settingsManager) { Package = p }) + .Transform(p => new PythonPackagesItemViewModel { Package = p }) .SortBy(vm => vm.Package.Name) .ObserveOn(SynchronizationContext.Current!) .Bind(Packages) @@ -107,17 +121,7 @@ private async Task Refresh() } else { - pyBaseInstall ??= new PyBaseInstall( - await pyInstallationManager.GetInstallationAsync( - PythonVersion ?? PyInstallationManager.Python_3_10_11 - ) - ); - - await using var venvRunner = await pyBaseInstall.CreateVenvRunnerAsync( - VenvPath, - workingDirectory: VenvPath.Parent, - environmentVariables: settingsManager.Settings.EnvironmentVariables - ); + await using var venvRunner = await CreateVenvRunnerAsync(); var packages = await venvRunner.PipList(); @@ -143,16 +147,7 @@ private async Task RefreshBackground() if (VenvPath is null) return; - pyBaseInstall ??= new PyBaseInstall( - await pyInstallationManager.GetInstallationAsync( - PythonVersion ?? PyInstallationManager.Python_3_10_11 - ) - ); - await using var venvRunner = await pyBaseInstall.CreateVenvRunnerAsync( - VenvPath, - workingDirectory: VenvPath.Parent, - environmentVariables: settingsManager.Settings.EnvironmentVariables - ); + await using var venvRunner = await CreateVenvRunnerAsync(); var packages = await venvRunner.PipList(); @@ -169,19 +164,83 @@ await pyInstallationManager.GetInstallationAsync( }); } + private async Task CreateVenvRunnerAsync() + { + if (VenvRunnerFactory is not null) + { + return await VenvRunnerFactory(); + } + + ArgumentNullException.ThrowIfNull(VenvPath); + + pyBaseInstall ??= new PyBaseInstall( + await pyInstallationManager.GetInstallationAsync( + PythonVersion ?? PyInstallationManager.Python_3_10_11 + ) + ); + + return await pyBaseInstall.CreateVenvRunnerAsync( + VenvPath, + workingDirectory: VenvPath.Parent, + environmentVariables: settingsManager.Settings.EnvironmentVariables + ); + } + /// /// Load the selected package's show info if not already loaded /// partial void OnSelectedPackageChanged(PythonPackagesItemViewModel? value) { - if (value is null) + // A load still waiting for its turn is no longer wanted once the selection moves on + extraInfoCts?.Cancel(); + extraInfoCts?.Dispose(); + extraInfoCts = null; + + if (value is null || (VenvPath is null && !Design.IsDesignMode)) + { + return; + } + + if (value.PipShowResult is not null || value.IsLoading) + { + return; + } + + var cts = new CancellationTokenSource(); + extraInfoCts = cts; + + LoadExtraInfoAsync(value, cts.Token) + .SafeFireAndForget(ex => + logger.LogWarning(ex, "Failed to load details for Python package {Name}", value.Package.Name) + ); + } + + private async Task LoadExtraInfoAsync( + PythonPackagesItemViewModel item, + CancellationToken cancellationToken + ) + { + try + { + await extraInfoLock.WaitAsync(cancellationToken); + } + catch (OperationCanceledException) { return; } - if (value.PipShowResult is null) + try + { + if (!ReferenceEquals(SelectedPackage, item) || item.PipShowResult is not null) + { + return; + } + + await item.LoadExtraInfo(CreateVenvRunnerAsync); + } + finally { - value.LoadExtraInfo(VenvPath!, pyBaseInstall!).SafeFireAndForget(); + extraInfoLock.Release(); } } diff --git a/StabilityMatrix.Core/Python/PipIndexResult.cs b/StabilityMatrix.Core/Python/PipIndexResult.cs index ea9e62f70..60b73efad 100644 --- a/StabilityMatrix.Core/Python/PipIndexResult.cs +++ b/StabilityMatrix.Core/Python/PipIndexResult.cs @@ -10,14 +10,20 @@ public partial record PipIndexResult public static PipIndexResult Parse(string output) { - var match = AvailableVersionsRegex().Matches(output); - - var versions = output + var versionsMatch = output .SplitLines() .Select(line => AvailableVersionsRegex().Match(line)) - .First(m => m.Success) - .Groups["versions"].Value - .Split( + .FirstOrDefault(m => m.Success); + + // Output without an "Available versions:" line (e.g. only warnings) has no versions to offer + if (versionsMatch is null) + { + return new PipIndexResult { AvailableVersions = [] }; + } + + var versions = versionsMatch + .Groups["versions"] + .Value.Split( new[] { ',' }, StringSplitOptions.TrimEntries | StringSplitOptions.RemoveEmptyEntries ) diff --git a/StabilityMatrix.Core/Python/PipShowResult.cs b/StabilityMatrix.Core/Python/PipShowResult.cs index 3df5ed388..8a2f6f865 100644 --- a/StabilityMatrix.Core/Python/PipShowResult.cs +++ b/StabilityMatrix.Core/Python/PipShowResult.cs @@ -37,12 +37,15 @@ public static PipShowResult Parse(string output) var indexOfLicense = GetIndexBySubstring(lines, "License:"); var indexOfLocation = GetIndexBySubstring(lines, "Location:"); - var licenseText = - indexOfLicense == -1 ? null : string.Join('\n', lines[indexOfLicense..indexOfLocation]); + // A multi-line license runs up to the "Location:" line. If that line is missing or comes + // first, only take the "License:" line itself. + var licenseEnd = indexOfLocation > indexOfLicense ? indexOfLocation : indexOfLicense + 1; + + var licenseText = indexOfLicense == -1 ? null : string.Join('\n', lines[indexOfLicense..licenseEnd]); if (indexOfLicense != -1) { - lines.RemoveRange(indexOfLicense, indexOfLocation - indexOfLicense); + lines.RemoveRange(indexOfLicense, licenseEnd - indexOfLicense); } var linesDict = new Dictionary(); diff --git a/StabilityMatrix.Tests/Core/PipIndexResultTests.cs b/StabilityMatrix.Tests/Core/PipIndexResultTests.cs new file mode 100644 index 000000000..b84c705a0 --- /dev/null +++ b/StabilityMatrix.Tests/Core/PipIndexResultTests.cs @@ -0,0 +1,35 @@ +using Microsoft.VisualStudio.TestTools.UnitTesting; +using StabilityMatrix.Core.Python; + +namespace StabilityMatrix.Tests.Core; + +[TestClass] +public class PipIndexResultTests +{ + [TestMethod] + public void Parse_ReadsAvailableVersions() + { + var output = """ + jax (0.4.30) + Available versions: 0.4.30, 0.4.29, 0.4.28 + INSTALLED: 0.4.26 + LATEST: 0.4.30 + """; + + var result = PipIndexResult.Parse(output); + + CollectionAssert.AreEqual(new[] { "0.4.30", "0.4.29", "0.4.28" }, result.AvailableVersions.ToArray()); + } + + [TestMethod] + public void Parse_WithoutAvailableVersionsLine_ReturnsNoVersions() + { + var output = """ + WARNING: pip index is currently an experimental command. It may be removed/changed in a future release without prior warning. + """; + + var result = PipIndexResult.Parse(output); + + Assert.AreEqual(0, result.AvailableVersions.Count); + } +} diff --git a/StabilityMatrix.Tests/Core/PipShowResultsTests.cs b/StabilityMatrix.Tests/Core/PipShowResultsTests.cs index 08ebf4d56..8e30508ab 100644 --- a/StabilityMatrix.Tests/Core/PipShowResultsTests.cs +++ b/StabilityMatrix.Tests/Core/PipShowResultsTests.cs @@ -181,4 +181,40 @@ public void TestEmptyInputThrowsFormatException() var input = ""; Assert.ThrowsException(() => PipShowResult.Parse(input)); } + + [TestMethod] + public void TestLicenseWithoutLocation() + { + var input = """ + Name: package-a + Version: 1.0.0 + License: MIT + Requires: numpy + """; + + var result = PipShowResult.Parse(input); + + Assert.AreEqual("package-a", result.Name); + Assert.AreEqual("License: MIT", result.License); + Assert.IsNull(result.Location); + CollectionAssert.AreEqual(new[] { "numpy" }, result.Requires); + } + + [TestMethod] + public void TestLocationBeforeLicense() + { + var input = """ + Name: package-a + Version: 1.0.0 + Location: /path/to/package + License: MIT + Requires: numpy + """; + + var result = PipShowResult.Parse(input); + + Assert.AreEqual("License: MIT", result.License); + Assert.AreEqual("/path/to/package", result.Location); + CollectionAssert.AreEqual(new[] { "numpy" }, result.Requires); + } } diff --git a/StabilityMatrix.UITests/PythonPackagesDialogTests.cs b/StabilityMatrix.UITests/PythonPackagesDialogTests.cs new file mode 100644 index 000000000..097d0cd32 --- /dev/null +++ b/StabilityMatrix.UITests/PythonPackagesDialogTests.cs @@ -0,0 +1,160 @@ +using Avalonia.Controls; +using Avalonia.Threading; +using Avalonia.VisualTree; +using Microsoft.Extensions.Logging.Abstractions; +using NSubstitute; +using StabilityMatrix.Avalonia.ViewModels.Dialogs; +using StabilityMatrix.Avalonia.Views.Dialogs; +using StabilityMatrix.Core.Helper; +using StabilityMatrix.Core.Models.FileInterfaces; +using StabilityMatrix.Core.Python; +using StabilityMatrix.Core.Services; + +namespace StabilityMatrix.UITests; + +/// +/// The Python Packages dialog loads a package's details (pip show / pip index) whenever the +/// selection changes, and searching moves the selection on almost every keystroke. +/// +public class PythonPackagesDialogTests +{ + private static readonly string[] PackageNames = + [ + "absl-py", + "accelerate", + "aiohttp", + "certifi", + "einops", + "filelock", + "idna", + "jax", + "jaxlib", + "jinja2", + "jsonmerge", + "jsonschema", + "kornia", + "lark", + "ml-dtypes", + "numpy", + "opt-einsum", + "pillow", + "pip", + "safetensors", + "scipy", + "torch", + "torchvision", + "transformers", + ]; + + [AvaloniaFact] + public async Task TypingInSearch_WhenPackageDetailsFailToLoad_DoesNotCrash() + { + var dialog = await PackagesDialog.OpenAsync(pipShow: name => + name.StartsWith('j') + ? throw new FileNotFoundException("uv not found") + : Task.FromResult(new PipShowResult { Name = name, Version = "1.0.0" }) + ); + + // Any exception escaping the details load is rethrown on the UI thread, which ends the app + await dialog.TypeSearchAsync("j", "ja", "jax", "ja", "j", ""); + + Assert.Equal("jax", dialog.Vm.SelectedPackage?.Package.Name); + Assert.All(dialog.Vm.Packages, p => Assert.False(p.IsLoading)); + dialog.Close(); + } + + [AvaloniaFact] + public async Task TypingInSearch_LoadsOnePackagesDetailsAtATime() + { + var running = 0; + var maxRunning = 0; + var dialog = await PackagesDialog.OpenAsync(pipShow: async name => + { + maxRunning = Math.Max(maxRunning, ++running); + await Task.Delay(150); + running--; + return new PipShowResult { Name = name, Version = "1.0.0" }; + }); + + await dialog.TypeSearchAsync("j", "js", "t", "to", "a"); + await dialog.SettleAsync(rounds: 20); + + Assert.Equal(1, maxRunning); + Assert.NotNull(dialog.Vm.SelectedPackage?.PipShowResult); + dialog.Close(); + } + + private sealed class PackagesDialog + { + private readonly Window window; + private readonly TextBox searchBox; + + public PythonPackagesViewModel Vm { get; } + + private PackagesDialog(PythonPackagesViewModel vm, Window window, TextBox searchBox) + { + Vm = vm; + this.window = window; + this.searchBox = searchBox; + } + + public static async Task OpenAsync(Func> pipShow) + { + var runner = Substitute.For(); + runner.PipList().Returns(PackageNames.Select(n => new PipPackageInfo(n, "1.0.0")).ToList()); + runner.PipShow(Arg.Any()).Returns(call => pipShow(call.Arg())); + runner + .PipIndex(Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(new PipIndexResult { AvailableVersions = ["1.0.0"] }); + + var vm = new PythonPackagesViewModel( + NullLogger.Instance, + Substitute.For(), + Substitute.For(), + Substitute.For() + ) + { + VenvPath = new DirectoryPath(Path.GetTempPath(), "venv"), + VenvRunnerFactory = () => Task.FromResult(runner), + }; + + var view = new PythonPackagesDialog { DataContext = vm }; + var window = new Window + { + Width = 1000, + Height = 700, + Content = view, + }; + window.Show(); + await vm.OnLoadedAsync(); + + var dialog = new PackagesDialog( + vm, + window, + view.GetVisualDescendants().OfType().First() + ); + await dialog.SettleAsync(); + return dialog; + } + + public async Task TypeSearchAsync(params string[] texts) + { + foreach (var text in texts) + { + searchBox.Text = text; + await SettleAsync(); + } + } + + public async Task SettleAsync(int rounds = 6) + { + for (var i = 0; i < rounds; i++) + { + await Task.Delay(50); + Dispatcher.UIThread.RunJobs(); + } + } + + public void Close() => window.Close(); + } +}