Replace RuntimeErrors with custom exceptions #486

t00sa · 2025-08-22T14:55:24Z

Swap all RuntimeError exceptions for custom fab exceptions using a class hierarchy derived from RuntimeError to minimise disruptive changes.

These changes fall into three broad areas:

creation of a new set of error classes in fab.errors and an associated set of tests
changes to the existing regression tests to:
- check the new message formats
- check the exceptions - which are still caught as RuntimeError to test the impact on calling functions - match the expected instance of the new error class
replacement of raise RuntimeError across the code base with the new error classes

While the change is complete and the test suite runs successfully in my environment, I still have a couple of outstanding questions that need to be addressed in review:

are the names of the new exceptions classes acceptable? The top level class names end with Error but the derived classes do not. Ending every class name with Error would make fab more consistent with python, at the cost of longer exception names
are there any potential clashes or issues with the in-flight baf PR?

Swap all RuntimeError exceptions for custom fab exceptions using a class hierarchy derived from RuntimeError to minimise distruptive changes.

Some minor flake8 problems had slipped in. This fixes them.

hiker

Thanks for this huge amount of work. I didn't expect that many classes to be used :)
FWIW, in psyclone we have around half the errors defined in some submodules (which helps avoid circular dependencies to some degree).

I have added a few comments. Could you also add a section to the development.rst file about errors, e.g. indicating that only Fab* errors should be raised, no standard errors.

hiker · 2025-08-26T02:25:56Z

source/fab/errors.py

+        self.tool = tool
+
+        # Check for name attributes rather than using isintance
+        # because Category and Tool ahve issues with circular


I don't think Category has a problem by itself, so I think that can be moved out of the TYPE_CHECKING test above (and fix the type here from string to be Category) - at least it worked for me).

Then I find the name tool confusing, since it can also be a category? Would it be worth storing both (tool and category, and leaving one of them None?? That might be useful for messages that a tool has the wrong category somewhere??). Or just call it tool_or_category?

I have thoughts on replacing the concept of categories with a more flexible system of capability tags. How does that interact with this issue?

I think my previous comment was a note to myself. It is an not an issue for this change.

source/fab/errors.py

hiker · 2025-08-26T02:34:19Z

source/fab/errors.py

+        if hasattr(tool, "name"):
+            self.name = tool.name
+        elif hasattr(tool, "_name"):
+            self.name = tool._name


Not sure what class this is, but please add a setter, don't access what is intended to be a private attribute.

A getter in this case? Agree that we shouldn't be accessing nominally private state.

hiker · 2025-08-26T02:43:49Z

source/fab/errors.py

+    """
+
+    def __init__(self, tool: Union["Category", "Tool", str], message: str) -> None:
+        self.tool = tool


Hmm - a coding style question: I prefer all attributes of a class to be 'private' (i.e. starting with _, and then add getter and/or setter as required). The big advantage (imho) of this is that the setter/getter will be documented, so a user can check out the supported methods in the documentation.
We recently investigated the performance impact of a getter (see stfc/PSyclone#3082), and found a 3 to 4% improvement when avoiding the getter. That's for getters that are being called 100.000s of time, I don't think we will notice a performance disadvantage of this in Fab.

I am ok leaving it as is (esp. since afaik it's not a pep8 requirement), but would prefer a clarification in the coding style :)

I too prefer private variables and getters/setters. It allows for validation on change of implementation without altering the API.

The @property modifier is available if the () of a function offends.

hiker · 2025-08-26T02:47:57Z

source/fab/errors.py

+        preserved for inspection by the caller.
+    """
+
+    def __init__(self, command: str) -> None:


Typing error: Union[list[str].str]

See Matthew's comment: we know that the command being passed to the constructor has a type of List[str], so we can change the type hinting and always execute the " ".join(command) line.

hiker · 2025-08-26T02:50:22Z

source/fab/errors.py

+        else:
+            raise ValueError(f"invalid command: {command}")
+
+        super().__init__(f"unable to execute {self.target}")


I would prefer quotes like '{self.target}' around the string. This helps identify white-space errors (e.g. when a user provides say touch abc as command, since we are running without shell, that line would result in unable to execute touch abc, which kind of hides the error that there is a space in the binary name.

hiker · 2025-08-26T02:52:01Z

source/fab/errors.py

+    """
+
+    def __init__(self, tool: str, reason: str, revision: Optional[str] = None) -> None:
+        self.tool = tool


I was confused at first, but then realised that this is not based on FabToolError. Nothing we can do about it, but maybe add a comment to explain this?

MatthewHambley · 2025-08-26T09:48:50Z

I'm about to start a review but I'll leave this comment here for immediate consideration. Are you aware of FabException in https://github.com/MetOffice/fab/blob/main/source/fab/__init__.py? Have you considered how this interacts with your change?

MatthewHambley

This is an extensive change which tidies up exception handling considerable. A few questions and suggestions included in-line.

In general, we have generated API documentation but is there anything which could be added (or removed) from the static documentation in light of this change?

MatthewHambley · 2025-08-26T09:51:57Z

source/fab/errors.py

+    """
+
+    def __init__(self, tool: Union["Category", "Tool", str], message: str) -> None:
+        self.tool = tool


I too prefer private variables and getters/setters. It allows for validation on change of implementation without altering the API.

The @property modifier is available if the () of a function offends.

MatthewHambley · 2025-08-26T09:53:11Z

source/fab/errors.py

+        self.tool = tool
+
+        # Check for name attributes rather than using isintance
+        # because Category and Tool ahve issues with circular


I have thoughts on replacing the concept of categories with a more flexible system of capability tags. How does that interact with this issue?

source/fab/errors.py

MatthewHambley · 2025-08-26T09:55:00Z

source/fab/errors.py

+        if hasattr(tool, "name"):
+            self.name = tool.name
+        elif hasattr(tool, "_name"):
+            self.name = tool._name


A getter in this case? Agree that we shouldn't be accessing nominally private state.

tests/unit_tests/test_cui_arguments.py

MatthewHambley · 2025-08-26T10:50:44Z

tests/unit_tests/test_errors.py

+        # Mock defines an internal name property, so special handling
+        # is required to reset the value to support testing
+        tool = Mock(_name="tool cc")
+        del tool.name
+        err = FabToolError(tool, "compiler message")
+        assert str(err) == "[tool cc] compiler message"


This seems alarming. Is this related to Joerg's comment about peeking at private state?

tests/unit_tests/test_errors.py

tests/unit_tests/tools/test_linker.py

hiker · 2025-10-10T14:30:37Z

@yaswant , I am a bit confused: I already did a review, and as far as I can see no work was done on the PR (and neither was there a reply to my comments). Additionally, the branch has conflicts ;)

yaswant · 2025-10-10T15:20:20Z

@hiker please ignore my assignment. You are right, Sam has not responded to your comments yet. Apologies for the confusion.

Replace the use of built-in python exceptions with Fab exception classes and AssertionErrors.

There appear to be two problems with the github continuous integration tests. The format of the argparse help for choices is quoted in some versions of python, so omit the choice strings from the output check. The git system tests need details of the current user to run. Add a step which adds some temporary user details.

t00sa

Clearly, some things I still need to tidy up. I'll get on with them tomorrow and update the PR when they're ready.

source/fab/steps/grab/prebuild.py

source/fab/errors.py

tests/unit_tests/test_cui_arguments.py

yaswant · 2025-10-22T18:21:57Z

.github/workflows/build.yml

+          git config --global user.email "fab"
+          git config --global user.name "Fab Testing"


I see you need this for git merge. There is no potential issue here, but does this have to be --global config? I know there is no strict validation here, but wondering if [email protected] as user.email would be better?

It probably doesn't have to be, but this is what git suggests when the configuration items don't exist

t00sa · 2025-10-23T10:25:01Z

I have addressed the comments from the previous reviews, the CI tests pass, and I've successfully used my branch to build skeleton in LFRic core using Jason's branch, so I think this ready for another round of reviewing.

MatthewHambley

While I think there's lots more work to do in the general area of error and exception handling, this is a good change and I'd rather see it on sooner and continue development than wait for it to be perfect.

A few comments left over from the previous review which haven't been addressed either with a change or a counter-comment.

In case you didn't know, if you from __future__ import annotate you don't need to quote type hints which have not yet been (or are currently being) defined.

MatthewHambley · 2025-11-17T14:29:30Z

source/fab/steps/archive_objects.py

    if not isinstance(ar, Ar):
        raise FabToolMismatch(ar.name, type(ar), "Ar")
-    output_fpath = str(output_fpath) if output_fpath else None
+    # output_fpath = str(output_fpath) if output_fpath else None


Commented-out code - This looks like development detritus.

MatthewHambley · 2025-11-17T14:45:17Z

source/fab/errors.py

+        self.tool = tool
+
+        # Check for name attributes rather than using isintance
+        # because Category and Tool ahve issues with circular


I think my previous comment was a note to myself. It is an not an issue for this change.

t00sa added 2 commits August 22, 2025 15:52

Replace RuntimeErrors with custom exceptions

d82dea4

Swap all RuntimeError exceptions for custom fab exceptions using a class hierarchy derived from RuntimeError to minimise distruptive changes.

Fix flake8 problems

7e378dd

Some minor flake8 problems had slipped in. This fixes them.

t00sa requested review from MatthewHambley and hiker August 22, 2025 15:35

t00sa added this to Fab Development Tracker Aug 22, 2025

t00sa moved this to In Review in Fab Development Tracker Aug 22, 2025

t00sa self-assigned this Aug 22, 2025

t00sa added the technical debt label Aug 22, 2025

t00sa linked an issue Aug 22, 2025 that may be closed by this pull request

Replace RuntimeError exceptions #421

Open

hiker requested changes Aug 26, 2025

View reviewed changes

MatthewHambley requested changes Aug 26, 2025

View reviewed changes

yaswant requested a review from hiker October 10, 2025 12:15

t00sa added 4 commits October 21, 2025 14:20

Remove old FabException references

e420c7d

Merge changes from main branch

bd90b22

Remove most built-in python exceptions

fff620d

Replace the use of built-in python exceptions with Fab exception classes and AssertionErrors.

t00sa requested a review from yaswant as a code owner October 22, 2025 14:22

t00sa commented Oct 22, 2025

View reviewed changes

yaswant reviewed Oct 22, 2025

View reviewed changes

t00sa added 2 commits October 23, 2025 09:03

Add note about ExceptionGroup issue to docstring

ebbb4bf

Apply remaining reviewer suggestions

7b52c47

t00sa requested a review from MatthewHambley October 23, 2025 10:25

t00sa added the Ready for review Indicating that a PR is ready to be reviewed. label Oct 23, 2025

MatthewHambley requested changes Nov 17, 2025

View reviewed changes

		git config --global user.email "fab"
		git config --global user.name "Fab Testing"

Replace RuntimeErrors with custom exceptions #486

Are you sure you want to change the base?

Replace RuntimeErrors with custom exceptions #486

Uh oh!

Conversation

t00sa commented Aug 22, 2025

Uh oh!

hiker 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!

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!

MatthewHambley commented Aug 26, 2025

Uh oh!

MatthewHambley 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hiker commented Oct 10, 2025

Uh oh!

yaswant commented Oct 10, 2025

Uh oh!

t00sa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t00sa commented Oct 23, 2025

Uh oh!

MatthewHambley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!