Raise the real import error when a thing cannot import #227
Conversation
This uses a WrapValidator to improve the handling of errors in an ImportString. The problem, identified in #222, is that `TypeError` and `ValueError` are treated specially by `pydantic`. This means that if either of those exceptions are raised during module import, we get a confusing validation error. My wrap validator ensures that we get either a sensible import-related error (e.g. moule not found, name not found in module) or a single clear error stating that an error occurred during module import. This does not dump a traceback, because doing so during validation tends to lead to very confusing messages. However, it should make it obvious that the problem is an unimportable module, and point someone in the right direction.
julianstirling
force-pushed
the
original-import-traceback
branch
from
55c61ef to
8bd000e
Compare
Barecheck - Code coverage reportTotal: 95.1%Your code coverage diff: 0.01% ▴ Uncovered files and lines
|
|
This seems to duplicate some of the logic that's already in I see that the error on the cli is unhelpful, because the traceback points to where the config is loaded. I think it might make sense to handle the validation error so the CLI can nicely tell the operator that the module you're trying to load is broken (without a traceback) and perhaps leave the post mortem for later and/or a log file. I appreciate it's most important to yank 0.0.12 and get a new version out, so if merging this is the best way to get us there, fair enough. I'd like to revisit this with less time pressure in the future though, if that moment ever comes... |
|
Duplicating logic is a shame. It would be good if we can extract the traceback from Pydantic, but it seems they record each of the errors to count them but not their tracebacks. I think if we can merge this we can make and MR to improve how it is done. Actually re-rasing outside validation does mean we need to do a lot more understanding of the way pydantic does errors and a lot more testing. |
julianstirling
force-pushed
the
original-import-traceback
branch
from
e88d55e to
837c6a7
Compare
|
So it turns our that pydantic does cache the error objects except for certain errors such as ModuleNotFound which are just strings. So this has been updated to use the tracebacks cached by Pydantic. |
rwb27
left a comment
rwb27
left a comment
There was a problem hiding this comment.
Looks good - let's get this in and we can think about a more elegant solution in the future. We might decide ImportString isn't for us after all...
|
Changing base was agreed with @rwb27 |
2 participants
This is a fix to better import errors. In the case that there is an import error, it actually imports the module again and raises that as a base exception to bypass pydantic capturing it and continuing. This way the error returned is as expected to the user.
Example
If I recreate the error I have changing one line in the server from:
downsampled_array_factor: int = lt.property(default=2)to
downsampled_array_factor: int = lt.property(2, default=2)This is in BaseCamera which is imported multiple times. In
better_import_errorsthis still fails with an unhelpful traceback that does not identify the line, or even really the file:With the changes in this PR this becomes
It is perhaps not perfect. But it does clearly identify the line at fault:
Example 2, Incorrect import
Just to verify that if the string itself if problematic we get a sensible traceback this is me changing
"system": "openflexure_microscope_server.things.system:OpenFlexureSystem",to (and
eis removed in the class name)"system": "openflexure_microscope_server.things.system:OpenFlxureSystem",