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?

hiker · 2025-08-26T02:31:45Z

source/fab/errors.py

+        # Check for name attributes rather than using isintance
+        # because Category and Tool ahve issues with circular
+        # dependencies
+        if hasattr(tool, "name"):


With Category imported directly, you can use some isinstance tests here. Otherwise I'd like a comment explaining what class you are accessing.

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]

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?

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

source/fab/errors.py

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


Suggested change

# because Category and Tool ahve issues with circular

# because Category and Tool have issues with circular

MatthewHambley · 2025-08-26T09:54:21Z

source/fab/errors.py

+if TYPE_CHECKING:
+    from fab.tools.category import Category
+    from fab.tools.tool import Tool


Don't like this. Is it due to the circular dependency you mention later?

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.

MatthewHambley · 2025-08-26T10:48:51Z

tests/unit_tests/test_cui_arguments.py


        captured = capsys.readouterr()
-        assert "error: fab file does not exist" in captured.err
+        assert "fab file does not exist" in captured.err


Where we are testing error messages I think we should test as much as possible. i.e. prefer equality over "in".

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?

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

tests/unit_tests/test_errors.py

+        """Test FabCommandError in various configurations."""
+
+        err = FabCommandError(["ls", "-l", "/nosuch"], 1, b"", b"ls: cannot", "/")
+        assert str(err) == "command 'ls -l /nosuch' returned 1"


Would it be easier to read if the sentence was restructured to put the variable length element last. e.g. "Return code 1 from command: ls -l /nosuch".

This way the eye knows where to go to for the error code and the start of the command.

MatthewHambley · 2025-08-26T10:54:37Z

tests/unit_tests/test_errors.py

+        assert str(err) == "[git] merge failed: conflicting source files"
+
+        err = FabSourceMergeError("git", "conflicting source files", "vn1.1")
+        assert str(err) == "[git] merge of vn1.1 failed: conflicting source files"


Maybe quote marks around revision?

MatthewHambley · 2025-08-26T10:56:27Z

tests/unit_tests/tools/test_linker.py

    with raises(RuntimeError) as err:
        linker.get_lib_flags("unknown")
-    assert str(err.value) == "Unknown library name: 'unknown'"
+    assert str(err.value) == "unknown library unknown"


This might benefit from a redrafting. Initial upper-case and maybe quote marks around library name.

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

	# because Category and Tool ahve issues with circular
	# because Category and Tool have issues with circular

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!

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!

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!

Uh oh!