Skip to content

Refuse to update a spectral library when none was given #2721

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
acesnik wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Refuse to update a spectral library when none was given #2721

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6fe5e32
Refuse to update a spectral library when none was given (#2291)
acesnik Aug 14, 2026
ab0b033
test: cover the spectral library update path directly
trishorts Aug 14, 2026
d4cf63d
Check for the spectral library before starting any work
acesnik Aug 16, 2026
a46a840
Build the .msp path with Path.Combine
acesnik Aug 16, 2026
c551b16
Refuse the update in the runner, so it reads as a notification
acesnik Aug 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions MetaMorpheus/TaskLayer/SearchTask/PostSearchAnalysisTask.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -817,6 +817,15 @@ private void WriteIndividualPeptideResults()
}
private void UpdateSpectralLibrary()
{
// SearchTask refuses this combination before searching, so reaching here without a library means
// some other caller built the parameters. Skip rather than throw: the search itself is finished
// and its results are already written.
if (Parameters.SpectralLibrary is null)
{
Warn("No spectral library was given, so there was nothing to update.");
return;
}

try
{
var peptidesForSpectralLibrary = FilteredPsms.Filter(Parameters.AllSpectralMatches,
Expand Down
9 changes: 9 additions & 0 deletions MetaMorpheus/TaskLayer/SearchTask/SearchTask.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,15 @@ protected override MyTaskResults RunSpecific(string OutputFolder, List<DbForTask
// load spectral libraries
var spectralLibrary = LoadSpectralLibraries(taskId, dbFilenameList);

// Checked here rather than where the library is updated, which happens after the whole search:
// there is nothing to update, and reporting that once the search has finished wastes the run.
if (SearchParameters.UpdateSpectralLibrary && spectralLibrary == null)

@trishorts trishorts Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The check lands nine lines too late to deliver what the PR describes.

var proteinLoadingTask = dbLoader.RunAsync(); already fired at line 171, and RunAsync is Task.Run(Run) (EngineLayer/MetaMorpheusEngine.cs:318) with no cancellation token — there is no CancellationToken overload anywhere in TaskLayer/EngineLayer, and DatabaseLoadingEngine.RunSpecific never checks GlobalVariables.StopLoops. Throwing here abandons that task: it runs the FASTA load, decoy generation and ScrambleHomologousDecoys to completion on a thread-pool thread after RunTask has unhooked FinishedSingleEngineHandler (MetaMorpheusTask.cs:685) and rethrown, and any exception it raises is never observed.

The check doesn't need any of that work. LoadSpectralLibraries is a pure filter over dbFilenameList — dbFilenameList.Where(p => p.IsSpectralLibrary), returning null when the list is empty (MetaMorpheusTask.cs:741-746), no I/O. So the whole test can move above line 170 and the cost of the mistake becomes nothing at all, rather than "a database load".

In practice the leak is small (thread-pool work, and WriteTargetDecoyFasta defaults to false so nothing is written), and SearchTask already abandons this task on other throw paths — :301 for an unknown isobaric mass tag. So this isn't a new class of problem. But it is the one line of the PR whose placement is the whole argument, and hoisting it is free.

{
throw new MetaMorpheusException(

@trishorts trishorts Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Worth knowing before you settle on throw: in the GUI this surfaces as a crash report prompt, not as a configuration message.

I traced it end to end. RunTask catches, writes a full dump into results.txt — system info, stack trace, TargetSite — and rethrows (MetaMorpheusTask.cs:683-700). EverythingRunnerEngine.Run has no try/catch around RunTask. MainWindow wires the runner with t.ContinueWith(EverythingRunnerExceptionHandler, TaskContinuationOptions.OnlyOnFaulted) (GUI/MainWindow.xaml.cs:1020), and that handler (:431-490) does not special-case MetaMorpheusException:

MessageBox.Show(message + "\n\nWould you like to report this crash?", "Runtime Error", MessageBoxButton.YesNo);

Answering Yes composes a mailto to mm_support@chem.wisc.edu with the stack trace and the toml. So a user who ticked one checkbox too many is told MetaMorpheus crashed and invited to file a bug about it. (Since you exercised this through a net10 harness on macOS, you'd have had no way to see that.)

The house channel for refusing a bad database/task combination is EverythingRunnerEngine.Run, which already does three of these with Warn(...) + FinishedAllTasks(...) + return — including one that inspects IsSpectralLibrary (EverythingRunnerEngine.cs:78-86, "Cannot proceed. No protein database files selected."). That gate runs before RunTask, has access to both the task list and the database list, and already has a test asserting on its warning string (Test/MyTaskTest.cs:839 MissingDbInSpectralLibrarySearch). Putting the refusal there would be earlier still, and would reach the user as a notification rather than a crash dialog.

Counter-argument, which is real: CalibrationTask.GenerateIndexes (CalibrationTask.cs:349-361) throws MetaMorpheusException for exactly this shape of impossible combination and is the newest precedent in the repo, and SearchTask.cs:301 does the same. So the inline throw isn't out of keeping. I'd still rather this one didn't read as a crash — either move it to the runner, or keep it here and teach EverythingRunnerExceptionHandler to present MetaMorpheusException as a message rather than a crash report (that second one is its own PR).

Either way, not something to fix silently — your call, and worth saying out loud in the PR description if you keep the throw.

"Updating a spectral library was requested, but no spectral library was given. Add one to " +
"the list of databases, or select writing a new spectral library instead of updating one.");
}

// write prose settings
ProseCreatedWhileRunning.Append("The following search settings were used: ");
ProseCreatedWhileRunning.Append($"{GlobalVariables.AnalyteType.GetDigestionAgentLabel()} = " + CommonParameters.DigestionParams.DigestionAgent + "; ");
Expand Down
49 changes: 49 additions & 0 deletions MetaMorpheus/Test/SpectralLibraryUpdateTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
using EngineLayer;
using NUnit.Framework;
using System.Collections.Generic;
using System.IO;
using TaskLayer;
using EngineLayer.DatabaseLoading;

namespace Test
{
[TestFixture]
public static class SpectralLibraryUpdateTests
{
/// <summary>
/// Issue #2291. Asking to update a spectral library without giving one used to run the whole search
/// and then throw NullReferenceException out of UpdateSpectralLibrary, which surfaced as the task
/// hanging on "Writing PSM results" with the exception only visible in results.txt afterwards.
///
/// The combination is refused before searching now, so the wasted run is what this pins: reaching
/// the check costs a database load, not a search.
/// </summary>
[Test]
public static void UpdatingASpectralLibraryWithoutOneIsRefusedBeforeSearching()
{
string outputFolder = Path.Combine(TestContext.CurrentContext.TestDirectory,
"SpectralLibraryUpdateTests", "NoLibrary");
Directory.CreateDirectory(outputFolder);

var task = new SearchTask();
task.SearchParameters.UpdateSpectralLibrary = true;

string database = Path.Combine(TestContext.CurrentContext.TestDirectory,
"TestData", "hela_snip_for_unitTest.fasta");
string spectra = Path.Combine(TestContext.CurrentContext.TestDirectory,
"TestData", "TaGe_SA_A549_3_snip.mzML");

var thrown = Assert.Throws<MetaMorpheusException>(() => task.RunTask(

@trishorts trishorts Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The test name and doc comment promise "before searching"; the assertions only prove "at all".

The three assertions are Assert.Throws<MetaMorpheusException> plus two substring checks on the message. Nothing observes that the search loop was skipped. The check could migrate anywhere upstream of PostSearchAnalysisTask — after the per-file search loop, say — and this test stays green, which is the half of the fix you describe as "as much the substance of the fix as the assertion is".

(To be precise about the one place it would catch: moving the throw into UpdateSpectralLibrary() itself would be swallowed by the try/catch at PostSearchAnalysisTask.cs:820/892-895 and Assert.Throws would fail on "But was: null" — the same failure you quote in the description. So the test does pin some earliness. It just doesn't pin "before the search".)

Cheapest fix that pins it, after the expected exception:

Assert.That(Directory.GetFiles(outputFolder, "*.psmtsv", SearchOption.AllDirectories), Is.Empty,
    "the refusal has to come before the search, not after it");
Assert.That(Directory.Exists(Path.Combine(outputFolder, "Individual File Results")), Is.False);

Two smaller things while you're in here:

  • The message assertions cover the diagnosis but not the remedy. Does.Contain("no spectral library was given") subsumes Does.Contain("spectral library"), and neither pins the half of the message that tells the user what to do about it — the string could be trimmed to the diagnosis and this stays green. One more substring on the "add one to the list of databases / write a new one instead" clause would cover it.
  • RunTask writes its toml to Directory.GetParent(output_folder)/"Task Settings" (MetaMorpheusTask.cs:627), i.e. SpectralLibraryUpdateTests/Task Settings/, one level above the folder you delete — so it survives every run. And the delete is the last statement rather than a finally, so a failing assertion leaves NoLibrary/ behind too. CalibrationTests.cs:273 and BinGenerationTest.cs:80 are the sibling pattern. (The test PR I'm sending you handles this with a [OneTimeTearDown] on the whole SpectralLibraryUpdateTests root.)

outputFolder,
new List<DbForTask> { new DbForTask(database, false) },
new List<string> { spectra },
"TestUpdateWithoutLibrary"));

Assert.That(thrown.Message, Does.Contain("spectral library"));
Assert.That(thrown.Message, Does.Contain("no spectral library was given").IgnoreCase,
"say what is missing, not just that something went wrong");

Directory.Delete(outputFolder, recursive: true);
}
}
}
Loading