Trigger more meaningful validation error messages when trainer registration failed#40
Trigger more meaningful validation error messages when trainer registration failed#40sfc-gh-lmerrick wants to merge 2 commits intomainfrom
Conversation
sfc-gh-caxu
left a comment
There was a problem hiding this comment.
Not obvious to me whether this change is needed because the error has been raised from get_registered_trainer
| raise ValueError(f"{trainer_name} is not a registered Trainer.") | ||
| raise KeyError(f"{trainer_name} is not a registered Trainer.") |
There was a problem hiding this comment.
It should be ValueError. ValueError suggests the function (get_registered_trainer) receives an invalid value. https://docs.python.org/3/library/exceptions.html#ValueError
There was a problem hiding this comment.
Is the registry not a key-value mapping? If it's a key-value mapping, I believe KeyError is the more specific, and thus more useful, exception to raise.
| except KeyError as e: | ||
| raise KeyError( |
There was a problem hiding this comment.
ValueError, see explanations. I think KeyError is rarely used and only for a local data type (e.g., a dict, set etc.)
In my tests, the rust-implemented |
|
Added a test. The test fails before the changes, but succeeds after. Previous test failure output: |
If a user fails to register their custom
Trainerclass, or they accidentally supply the incorrect trainer type in the trainer config, validation can proceed in a difficult-to-debug manner. This PR introduces more explicit error messages that trigger in this case and can help the user understand the root cause of the issue faster.