Better import error #389
Better import error #389
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #389 +/- ##
==========================================
+ Coverage 73.35% 73.75% +0.39%
==========================================
Files 22 22
Lines 3318 3296 -22
==========================================
- Hits 2434 2431 -3
+ Misses 884 865 -19 ☔ View full report in Codecov by Sentry. |
| import unittest | ||
|
|
||
| # isort: off | ||
| if sys.version_info[0] == 2: |
There was a problem hiding this comment.
wow, some py2 heritage here, good catch
pyoptsparse/pyOpt_utils.py
Outdated
| if path is not None: | ||
| warnings.warn( | ||
| f"{module_name} module could not be imported from {path}.", | ||
| ImportWarning, |
There was a problem hiding this comment.
Not related directly to this PR, but is testflo raising ImportWarning by default? I tested just this function locally and the string above (which can be pretty useful) is not printed out by default - see here
There was a problem hiding this comment.
Hmm this is a good point. Maybe I will switch to UserWarning? I hate how a lot of warnings are not displayed by default.
There was a problem hiding this comment.
Yeah, that's the default but we can set it explicitly.
| module = str(e) | ||
| finally: | ||
| sys.path = orig_path | ||
| return module |
There was a problem hiding this comment.
Is there a reason why we delay throwing the ImportError? If we can't import the module, why not throw the error with the descriptive message here instead of a warning?
Do we need this "soft fail" type behavior somewhere that I missed? Otherwise I'm in favor of just throwing the ImportError here with the same message and only returning a module if the import is successful. But again, let me know if I missed something that makes this necessary.
There was a problem hiding this comment.
Yeah it's because this is executed as part of the init process, and we don't necessarily want to raise an error. But if they try to instantiate the class and the compiled library is missing, we throw the error.
There was a problem hiding this comment.
Okay that makes sense. I forgot that python tries to import anything that's involved in the init process. I'll approve this now.
| if nlpqlp is None: | ||
| if raiseError: | ||
| raise Error("There was an error importing the compiled nlpqlp module") | ||
| if isinstance(nlpqlp, str) and raiseError: |
There was a problem hiding this comment.
Related to my other comment, why check for strings and then throw the same error that we could throw in the try_import_compiled_module_from_path function?
There was a problem hiding this comment.
This is in the class init, so it's only executed when the class is instantiated.
Purpose
Closes #187
Expected time until merged
Few days
Type of change
Testing
No new tests, no new change in behaviour other than
ImportErroris now raised instead of genericError.Checklist
flake8andblackto make sure the Python code adheres to PEP-8 and is consistently formattedfprettifyor C/C++ code withclang-formatas applicable