Add better error handling by dlcaballero16 · Pull Request #682 · neutrons/SNAPRed

dlcaballero16 · 2026-06-08T18:51:28Z

Description of work

Originally this story was to fix an issue with reducing livedata, but I was not able to recreate the defect. These changes should hopefully prevent the error from happening, and at the very least improve the error message that gets reported by SNAPRed. Also, a CIS script was added that will go through the calibration folder and add cycle info as needed. Cycle info is required for reduction, but if no cycle data is present, the reduction result will be marked as diagnostic.

Explanation of work

In LocalDataService.py, attempting to read live metadata is now run in a try/except block and an error message about missing PV info is reported.
In StateValidationException.py, the actual exception string is added to the error message in the case that the exception type is not a FileNotFoundError or a PermissionError.
A check was added in the ReductionService so that if no cycle data is present, then a warning box pops up if the user wants to continue. If they chose to continue, then the resulting reduction gets marked as diagnostic.

To test

Dev testing

Make sure unit tests pass and also see CIS testing.

CIS testing

Open mantid workbench and load the new update_SNAPInstPrm_cycle.py script. There are 3 user settings in the script defined near the top:

DRY_RUN - leave this set to true unless you actually want the script to update the files!
OVERWRITE_EXISTING - leave this to false unless you want to overwrite previous cycle data
CYCLE_METADATA - this is where you define the cycle data and how the SNAPInstPrm.json's get updated. It is a dictionary of dictionaries where each dictionary is keyed on the first run of the cycle, and the value is the rest of the info required to fill out a cycle object.

You should be able to run the script as is and see output showing how the SNAPInstPrm for each version folder found will be updated. Try adding more cycles to the CYCLE_METADATA and make sure the script still works.

To test the cycle continue warning, attempt to run reduction with no cycle data present. You should see a continue warning pop up saying you are missing cycle info. If you click continue and finish reduction, your reduction output workspaces should be marked as diagnostic. Now, try reduction again with valid cycle info, and you should not see a pop up about cycle info and now see that the workspace will not be marked as diagnostic (given that running reduction does not already lead to diagnostic output).

Link to EWM item

EWM#16396

Verification

the author has read the EWM story and acceptance critera
the reviewer has read the EWM story and acceptance criteria
the reviewer certifies the acceptance criteria below reflect the criteria in EWM

Acceptance Criteria

This list is for ease of reference, and does not replace reading the EWM story as part of the review. Verify this list matches the EWM story before reviewing.

acceptance criterion 1
acceptance criterion 2

script to update cycle info

codecov · 2026-06-08T19:23:43Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.50%. Comparing base (be1553c) to head (a7b0fdc).
⚠️ Report is 1 commits behind head on next.

Additional details and impacted files

@@            Coverage Diff             @@
##             next     #682      +/-   ##
==========================================
+ Coverage   96.47%   96.50%   +0.03%     
==========================================
  Files          79       79              
  Lines        7258     7270      +12     
==========================================
+ Hits         7002     7016      +14     
+ Misses        256      254       -2

Flag	Coverage Δ
integration	`48.77% <22.22%> (-0.07%)`	⬇️
unittests	`96.23% <100.00%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

script

data is missing

ekapadi

So far, there are only some minor issues, with putting stuff in DataFactoryService that should actually be routed from LocalDataService. I also would like you to take a deeper look at the exception flow in these two cases: (Indexer -> InstrumentParameters -> ...) and (<live-data fallback> -> PV-file -> <run metadata>, and etc.). I still am not entirely satisfied that we've completely fixed the misleading messages issue.

For "testing", it would be a huge help for you to please explain how a developer (not a CIS) should test this? I'm probably not going to run the migration script, so I'm at a bit of a loss. I think for this, just explain what I need to do to set up my directories so that they will trigger or not-trigger these changes?

ekapadi · 2026-06-30T10:22:51Z

        return self.lookupService.readInstrumentConfig(runId)

-    def getCycleID(self, runNumber: str) -> str:
+    def cycleInfoExists(self, runNumber: str) -> bool:


SOP: these need to call LocalDataService methods. They shouldn't be ever fully implemented in DataFactoryService. Sometimes the backend might need to access these methods directly, and in that case you now have no way to do that from LocalDataService. Look at the other surrounding methods, and you'll see what the pattern is.

ekapadi · 2026-06-30T10:23:41Z

        if not success:
-            metadata = self.readLiveMetadata()
+            # No local PVFile for `runNumber`, but a live-data connection exists: fall back to the live run.
+            #   If the fallback itself fails, make clear that (1) there's no local data for the requested run,


This took me a while to figure out, but in the end, I agree with what you've done. Gosh all of this is complicated! :)

ekapadi · 2026-06-30T10:25:15Z

+            self.message = (
+                "Instrument State for given Run Number is invalid!\n\n"
+                + f"{exceptionStr}\n\n"
+                + "(See logs for details.)"


This "see logs for details" statement needs to go away. Can we just put all of the information here? I'm especially fond of the case "see logs for details", and then there's no additional information in the logs -- this is the usual case! :(

ekapadi · 2026-06-30T10:28:09Z

@@ -250,6 +261,8 @@ def reduction(self, request: ReductionRequest):
        )
        isDiagnostic = workspaceMetadata.diffcalState != DiffcalStateMetadata.EXISTS


Can't all of this just be one short-circuit logical statement? I'm not sure that's really better, but try it and see?

ekapadi · 2026-06-30T10:29:04Z

+# Each instrument-parameter version lives in its own folder:
+#     <instrument.parameters.home>/v_XXXX/SNAPInstPrm.json
+# This walks those folders and, for each file, derives the cycle from the file's own
+# `indexEntry.appliesTo` run range:


Please justify why we didn't just extend / fix the original instrument-parameters migration script. This is after all just an instrument-parameters migration?!

ekapadi · 2026-06-30T10:30:20Z

        self.instance.lookupService.readInstrumentParameters.assert_called_once_with("200")

    def test_getCycleID_no_cycle(self):
+        # Missing cycle info is not an error: a fallback cycle ID is returned so reduction


Same issue as mentioned at DataFactoryService. You are testing what should be LocalDataService methods. For DataFactoryService these would be captured via the generic testing procedure, which automatically triggers the sub-call. (Take a lookk and you'll see what I mean, although it's a bit abstract, and I don't think we use it consistently.)

ekapadi · 2026-06-30T10:32:56Z

+    # A `FileNotFoundError` that isn't a "No PVFile exists" error should be re-raised as-is.
+    runNumber = "12345"
+    instance = LocalDataService()
+    instance._readPVFile = mock.Mock(side_effect=FileNotFoundError("some other file is missing"))


Wouldn't it be incorrect to raise a FileNotFoundError when you're unable to construct the instrument-parameters due to an Indexer error? This is an example of the exception-flow issue that I mentioned previously (in my slack message)!

ekapadi · 2026-06-30T10:34:04Z

-    with pytest.raises(
-        StateValidationException, match=r"Instrument State for given Run Number is invalid! \(See logs for details\.\)"
-    ):
+    with pytest.raises(StateValidationException, match=r"Instrument State for given Run Number is invalid!") as excinfo:


YES! Get rid of the stupid "See logs for details". Could we get rid of it everywhere else as well? :)

dlcaballero16 and others added 6 commits June 8, 2026 14:30

Updated state validation exception to have better error handling, added

6734d43

script to update cycle info

Use token to get gitlab test files

a2c900a

Fix typo

075351a

Update pixi.lock file

fa85400

Merge branch 'next' into fix-livedata-error-reporting

2ef1872

Update pixi.lock and pip version to satisfy pip-audit

f12b76d

dlcaballero16 added 5 commits June 8, 2026 16:40

Added tests for coverage

d67dbb9

Made cycle optional if run number is below threshold, moved update cycle

34fe4e5

script

Removed model validator, instead mark reduction as diagnostic if cycle

43f7a75

data is missing

Merge remote-tracking branch 'origin' into fix-livedata-error-reporting

99493a5

Attempting to fix CI issues

a7b0fdc

ekapadi reviewed Jun 30, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add better error handling#682

Add better error handling#682
dlcaballero16 wants to merge 11 commits into
nextfrom
fix-livedata-error-reporting

dlcaballero16 commented Jun 8, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Jun 8, 2026 •

edited

Loading

Uh oh!

ekapadi left a comment

Uh oh!

ekapadi Jun 30, 2026

Uh oh!

ekapadi Jun 30, 2026

Uh oh!

ekapadi Jun 30, 2026

Uh oh!

ekapadi Jun 30, 2026

Uh oh!

ekapadi Jun 30, 2026

Uh oh!

ekapadi Jun 30, 2026

Uh oh!

ekapadi Jun 30, 2026

Uh oh!

ekapadi Jun 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -250,6 +261,8 @@ def reduction(self, request: ReductionRequest):
		)
		isDiagnostic = workspaceMetadata.diffcalState != DiffcalStateMetadata.EXISTS

Uh oh!

Conversation

dlcaballero16 commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of work

Explanation of work

To test

Dev testing

CIS testing

Link to EWM item

Verification

Acceptance Criteria

Uh oh!

codecov Bot commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ekapadi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dlcaballero16 commented Jun 8, 2026 •

edited

Loading

codecov Bot commented Jun 8, 2026 •

edited

Loading