refactor: replace ValueError and KeyError with custom exception types by jinukuntlaakhilakumargoud-web · Pull Request #385 · PowerGridModel/power-grid-model-io

jinukuntlaakhilakumargoud-web · 2026-03-02T16:37:58Z

Closes #384

This PR implements the custom error types discussed in Issue #384.

Replaced generic ValueError and KeyError exceptions with specific, custom-built exception types in the conversion module.
Introduced exceptions.py featuring:

ComponentAlreadyExistsError(ValueError)
- ComponentNotFoundError(KeyError)
- InvalidComponentTypeError(ValueError)
- InvalidDataFormatError(ValueError)

mgovers

Hi @jinukuntlaakhilakumargoud-web,

Thank you for your contribution! Overall, it seems to be in a good state, but I will not be able to do a full review today. Here's some initial feedback on a couple things that caught my eye.

Hope that helps.

src/power_grid_model_io/__init__.py

src/power_grid_model_io/errors.py

furqan463 · 2026-03-02T17:41:19Z

Need to update tests as well, as the tests are currently expecting ValueError etc.

jinukuntlaakhilakumargoud-web · 2026-03-02T19:14:29Z

Hi @mgovers and @furqan463,

Thank you for the feedback! I have pushed a new commit that addresses all the points:

Removed the custom exception exports from __init__.py.
1. Renamed exceptions.py to errors.py.
1. Updated the tests in test_tabular_converter.py to expect the new custom exception types (InvalidComponentTypeError, ComponentNotFoundError, InvalidDataFormatError) where appropriate, replacing the old ValueError and KeyError assertions.
  All 749 tests are now passing locally with 98% coverage! Let me know if anything else is needed.

furqan463 · 2026-03-02T23:18:41Z

Hi @mgovers and @furqan463,

Thank you for the feedback! I have pushed a new commit that addresses all the points:

Removed the custom exception exports from __init__.py.

Renamed exceptions.py to errors.py.

Updated the tests in test_tabular_converter.py to expect the new custom exception types (InvalidComponentTypeError, ComponentNotFoundError, InvalidDataFormatError) where appropriate, replacing the old ValueError and KeyError assertions.
All 749 tests are now passing locally with 98% coverage! Let me know if anything else is needed.

Great, and please make signed commits to avoid DCO failure.

…closes PowerGridModel#384) Signed-off-by: jinukuntlaakhilakumargoud-web <jinukuntlaakhilakumargoud@gmail.com>

- Updated tests in test_tabular_converter to expect InvalidDataFormatError and InvalidComponentTypeError - Un-exported custom exception classes from __init__.py as per maintainer feedback - Renamed exceptions.py to errors.py to prevent potential shadowing - Reverted NotImplementedError replacements in test_serialize_data Signed-off-by: jinukuntlaakhilakumargoud-web <jinukuntlaakhilakumargoud@gmail.com>

jinukuntlaakhilakumargoud-web · 2026-03-03T04:06:26Z

@furqan463 Done! I've rebased the commits and added the Signed-off-by line to fix the DCO failure. Force-pushed the updates, so the DCO checks should be passing now.

src/power_grid_model_io/converters/pandapower_converter.py

src/power_grid_model_io/converters/tabular_converter.py

src/power_grid_model_io/converters/vision_excel_converter.py

src/power_grid_model_io/__init__.py

jinukuntlaakhilakumargoud-web · 2026-03-03T10:39:01Z

I have addressed all the comments:

Replaced generic errors with InvalidDatasetTypeError, MappingNotFoundError, and AttributeNotFoundError in the conversion module.
- Reverted unintended changes in src/power_grid_model_io/__init__.py.
- Reverted the functional change in tabular_converter.py (line 825) to avoid modifying the logic.
- Verified all unit tests locally (297 passed).
- Fixed the DCO failure by amending the commits with -s.
  Please let me know if any further changes are needed.

furqan463 · 2026-03-03T13:01:12Z

I have addressed all the comments:

Replaced generic errors with InvalidDatasetTypeError, MappingNotFoundError, and AttributeNotFoundError in the conversion module.

Reverted unintended changes in src/power_grid_model_io/__init__.py.

Reverted the functional change in tabular_converter.py (line 825) to avoid modifying the logic.

Verified all unit tests locally (297 passed).

Fixed the DCO failure by amending the commits with -s.
Please let me know if any further changes are needed.

Run pre-commit to fix code quality issues
uv run pre-commit run --all
Copy licence information from any of the .py file and put it at top of the errors.py file to fix reuse compliance.

Signed-off-by: jinukuntlaakhilakumargoud-web <jinukuntlaakhilakumargoud@gmail.com>

mgovers · 2026-03-03T14:00:05Z

import_diagnostic.txt

these auto-generated files should not be present in the git repo

Thanks for catching that! I've removed the auto-generated diagnostic files (import_diagnostic.txt and export_diagnostic.txt) from the repository and updated .gitignore so they aren't accidentally committed again. The changes have been pushed to the PR!

@jinukuntlaakhilakumargoud-web text files are still there.

@furqan463 You're right, I accidentally committed those test_collect_output*.txt files. I have removed them now, so no extra text files should remain.

Signed-off-by: jinukuntlaakhilakumargoud-web <jinukuntlaakhilakumargoud@gmail.com>

furqan463 · 2026-03-04T04:59:02Z

@jinukuntlaakhilakumargoud-web no need to add these to .gitignore.
instead, delete the text files and then commit.

mgovers · 2026-03-04T07:54:59Z

.gitignore

did the line endings change? were they \r\n before instead of plain \n (in that case, you can keep it as-is) or did you change them to \r\n?

I've fixed the line endings back to \n. Thanks for catching that.

mgovers · 2026-03-04T07:56:02Z

.gitignore

+docs/Makefile
+
+# Ignore diagnostic output
+*diagnostic.txt


were the test_collect_output files auto-generated or did you manually add them yourself? if the former, please also add test_collect_output*.txt here. If the latter, please do not add it here.

The test_collect_output*.txt files were manually generated when I was redirecting test output and were accidentally committed. I have removed them from the repository entirely and reverted the change to .gitignore that added them

figueroa1395

Mainly nitpicks. Thanks for the contribution!

figueroa1395 · 2026-03-04T10:32:43Z

.gitignore

+# Ignore diagnostic output
+*diagnostic.txt


I believe this is very specific to your workflow. Can you remove it and just manually delete or not commit these files?

Thank you for the feedback. I've removed the diagnostic file entries from .gitignore.

figueroa1395 · 2026-03-04T10:44:07Z

src/power_grid_model_io/errors.py

+
+
+class ComponentNotFoundError(KeyError, PowerGridModelIoError):
+    """Raised when a required component or attribute is missing."""


Let's separate components from attributes.

Suggested change

"""Raised when a required component or attribute is missing."""

"""Raised when a required component is missing."""

Good catch. I've updated the docstring to only refer to components, as suggested.

figueroa1395 · 2026-03-04T10:45:53Z

src/power_grid_model_io/errors.py

+class AttributeNotFoundError(ComponentNotFoundError):
+    """Raised when an attribute is missing from a component/dataset."""


With reference to https://github.com/PowerGridModel/power-grid-model-io/pull/385/changes#r2883076143. Let's keep the attribute related error independent:

Suggested change

class AttributeNotFoundError(ComponentNotFoundError):

"""Raised when an attribute is missing from a component/dataset."""

class AttributeNotFoundError(KeyError, PowerGridModelIoError):

"""Raised when a required component attribute is missing."""

Thank you for the suggestion. I've updated AttributeNotFoundError to inherit from KeyError, PowerGridModelIoError directly.

figueroa1395 · 2026-03-04T10:50:35Z

src/power_grid_model_io/errors.py

+
+
+class InvalidDatasetTypeError(ValueError, PowerGridModelIoError):
+    """Raised when an invalid dataset type (e.g. input/output) is encountered."""


Suggested change

"""Raised when an invalid dataset type (e.g. input/output) is encountered."""

"""Raised when an invalid dataset type (e.g. other than input, output, or update) is encountered."""

Updated the docstring as suggested. Thank you.

figueroa1395 · 2026-03-04T11:00:50Z

src/power_grid_model_io/converters/tabular_converter.py

+            raise InvalidDatasetTypeError(
+                f"Invalid component type '{component_str}' or data type '{data_type_str}'"
+            ) from ex


This exception lies in between InvalidDatasetTypeError and InvalidComponentTypeError. Can you split it?

I've split the exception handling to raise InvalidDatasetTypeError for invalid data types and InvalidComponentTypeError for invalid component types separately.

figueroa1395 · 2026-03-04T11:12:51Z

src/power_grid_model_io/converters/pandapower_converter.py


        if ComponentType.line in self.pgm_input_data:
-            raise ValueError("Line component already exists in pgm_input_data")
+            raise ComponentAlreadyExistsError("Line component already exists in pgm_input_data")


Seeing all the repetition here for the messages when raising ComponentAlreadyExistsError, maybe it's a good idea if we could do something like:

Suggested change

raise ComponentAlreadyExistsError("Line component already exists in pgm_input_data")

raise ComponentAlreadyExistsError("Line", "pgm_input_data")

By modifying ComponentAlreadyExistsError to something along the lines of:

class ComponentAlreadyExistsError(ValueError, PowerGridModelIoError): def __init__(self, component: str, dataset: str): super().__init__(f"'{component}' component already exists in {dataset}")

I didn't recommend using ComponentType because that would probably be a breaking change, but please take a look.

Similar suggestion applies below in this file.

Great suggestion. I've updated ComponentAlreadyExistsError to accept component and dataset arguments, and updated all call sites accordingly.

figueroa1395 · 2026-03-04T11:14:59Z

src/power_grid_model_io/converters/pandapower_converter.py

        key = (pp_table, name)
        if key in self.idx_lookup:
-            raise KeyError(f"Indexes for '{key}' already exist!")
+            raise ComponentAlreadyExistsError(f"Indexes for '{key}' already exist!")


I don't think this one belongs to this error type.

Agreed. I've introduced IndexAlreadyExistsError for this case instead.

figueroa1395 · 2026-03-04T11:16:53Z

src/power_grid_model_io/converters/pandapower_converter.py

                    return {"table": table, "name": name, "index": indices[pgm_id]}
                return {"table": table, "index": indices[pgm_id]}
-        raise KeyError(pgm_id)
+        raise ComponentNotFoundError(pgm_id)


Neither does this one. Perhaps this one and the above can be put into something like IndexToComponentNotFoundError or something similar.

My suggestion is a bit long, but that's the idea.

Thank you for the suggestion. I've introduced IndexToComponentNotFoundError for this case.

figueroa1395 · 2026-03-04T11:18:31Z

src/power_grid_model_io/converters/pgm_json_converter.py

                    obj_id = obj["id"]
                    if not isinstance(obj_id, int):
-                        raise ValueError(f"Invalid 'id' value for {component.value} {data_type.value} data")
+                        raise InvalidDataFormatError(f"Invalid 'id' value for {component.value} {data_type.value} data")


This one perhaps belongs to what I suggested in https://github.com/PowerGridModel/power-grid-model-io/pull/385/changes#r2883220795 instead.

Noted. I've kept InvalidDataFormatError for this case as it relates to data format validation rather than component type issues.

…et) signature and fix test assertions - Updated all ComponentAlreadyExistsError calls in pandapower_converter.py to use the new (component, dataset) constructor signature - Replaced ValueError with ComponentAlreadyExistsError in _pp_load_elements_output - Updated pytest.raises assertions in test_pandapower_converter_input.py and test_pandapower_converter_output.py to use ComponentAlreadyExistsError without match arguments - All 207 pandapower converter tests pass

mgovers reviewed Mar 2, 2026

View reviewed changes

src/power_grid_model_io/__init__.py Show resolved Hide resolved

src/power_grid_model_io/errors.py Show resolved Hide resolved

mgovers added the improvement Existing functionality, but better, faster, stronger label Mar 2, 2026

mgovers changed the title ~~refactor: replace ValueError and KeyError with custom exception types (closes #384)~~ refactor: replace ValueError and KeyError with custom exception types Mar 2, 2026

mgovers assigned jinukuntlaakhilakumargoud-web Mar 2, 2026

jinukuntlaakhilakumargoud-web added 2 commits March 3, 2026 09:23

refactor: replace ValueError and KeyError with custom exception types (…

323aeed

…closes PowerGridModel#384) Signed-off-by: jinukuntlaakhilakumargoud-web <jinukuntlaakhilakumargoud@gmail.com>

jinukuntlaakhilakumargoud-web force-pushed the fix-generic-errors branch from b48efe5 to 60f40f0 Compare March 3, 2026 03:54

mgovers reviewed Mar 3, 2026

View reviewed changes

jinukuntlaakhilakumargoud-web force-pushed the fix-generic-errors branch from ceef25a to adb56e7 Compare March 3, 2026 10:10

jinukuntlaakhilakumargoud-web force-pushed the fix-generic-errors branch from adb56e7 to b0b6124 Compare March 3, 2026 13:28

refactor: use specialized exception types and revert unintended changes

367b993

Signed-off-by: jinukuntlaakhilakumargoud-web <jinukuntlaakhilakumargoud@gmail.com>

jinukuntlaakhilakumargoud-web force-pushed the fix-generic-errors branch from b0b6124 to 367b993 Compare March 3, 2026 13:32

mgovers reviewed Mar 3, 2026

View reviewed changes

chore: remove auto-generated diagnostic files

86ed451

Signed-off-by: jinukuntlaakhilakumargoud-web <jinukuntlaakhilakumargoud@gmail.com>

mgovers reviewed Mar 4, 2026

View reviewed changes

chore: remove accidental text files and fix .gitignore line endings

3a0c441

figueroa1395 reviewed Mar 4, 2026

View reviewed changes



		class ComponentNotFoundError(KeyError, PowerGridModelIoError):
		"""Raised when a required component or attribute is missing."""

	"""Raised when a required component or attribute is missing."""
	"""Raised when a required component is missing."""

		class AttributeNotFoundError(ComponentNotFoundError):
		"""Raised when an attribute is missing from a component/dataset."""



		class InvalidDatasetTypeError(ValueError, PowerGridModelIoError):
		"""Raised when an invalid dataset type (e.g. input/output) is encountered."""

	raise ComponentAlreadyExistsError("Line component already exists in pgm_input_data")
	raise ComponentAlreadyExistsError("Line", "pgm_input_data")

Conversation

jinukuntlaakhilakumargoud-web commented Mar 2, 2026 • edited by mgovers Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgovers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

furqan463 commented Mar 2, 2026

Uh oh!

jinukuntlaakhilakumargoud-web commented Mar 2, 2026

Uh oh!

furqan463 commented Mar 2, 2026

Uh oh!

jinukuntlaakhilakumargoud-web commented Mar 3, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jinukuntlaakhilakumargoud-web commented Mar 3, 2026

Uh oh!

furqan463 commented Mar 3, 2026

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!

furqan463 commented Mar 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jinukuntlaakhilakumargoud-web Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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

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

jinukuntlaakhilakumargoud-web commented Mar 2, 2026 •

edited by mgovers

Loading

jinukuntlaakhilakumargoud-web Mar 4, 2026 •

edited

Loading