Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Better import error #389
Better import error #389
Changes from 11 commits
701fa90
a9fc028
352db37
1fc3dfd
e72769a
de95d6e
91e8759
6246064
7e916b2
0b1714d
e1f3207
e36655c
cbfa664
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 13 in pyoptsparse/pyALPSO/pyALPSO.py
Codecov / codecov/patch
pyoptsparse/pyALPSO/pyALPSO.py#L13
Check warning on line 29 in pyoptsparse/pyALPSO/pyALPSO.py
Codecov / codecov/patch
pyoptsparse/pyALPSO/pyALPSO.py#L29
Check warning on line 16 in pyoptsparse/pyCONMIN/pyCONMIN.py
Codecov / codecov/patch
pyoptsparse/pyCONMIN/pyCONMIN.py#L16
Check warning on line 20 in pyoptsparse/pyCONMIN/pyCONMIN.py
Codecov / codecov/patch
pyoptsparse/pyCONMIN/pyCONMIN.py#L19-L20
Check warning on line 34 in pyoptsparse/pyCONMIN/pyCONMIN.py
Codecov / codecov/patch
pyoptsparse/pyCONMIN/pyCONMIN.py#L34
Check warning on line 8 in pyoptsparse/pyIPOPT/pyIPOPT.py
Codecov / codecov/patch
pyoptsparse/pyIPOPT/pyIPOPT.py#L8
Check warning on line 16 in pyoptsparse/pyIPOPT/pyIPOPT.py
Codecov / codecov/patch
pyoptsparse/pyIPOPT/pyIPOPT.py#L16
Check warning on line 28 in pyoptsparse/pyIPOPT/pyIPOPT.py
Codecov / codecov/patch
pyoptsparse/pyIPOPT/pyIPOPT.py#L27-L28
Check warning on line 47 in pyoptsparse/pyIPOPT/pyIPOPT.py
Codecov / codecov/patch
pyoptsparse/pyIPOPT/pyIPOPT.py#L47
Check warning on line 16 in pyoptsparse/pyNLPQLP/pyNLPQLP.py
Codecov / codecov/patch
pyoptsparse/pyNLPQLP/pyNLPQLP.py#L16
Check warning on line 20 in pyoptsparse/pyNLPQLP/pyNLPQLP.py
Codecov / codecov/patch
pyoptsparse/pyNLPQLP/pyNLPQLP.py#L19-L20
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in the class init, so it's only executed when the class is instantiated.
Check warning on line 34 in pyoptsparse/pyNLPQLP/pyNLPQLP.py
Codecov / codecov/patch
pyoptsparse/pyNLPQLP/pyNLPQLP.py#L34
Check warning on line 6 in pyoptsparse/pyNSGA2/pyNSGA2.py
Codecov / codecov/patch
pyoptsparse/pyNSGA2/pyNSGA2.py#L6
Check warning on line 15 in pyoptsparse/pyNSGA2/pyNSGA2.py
Codecov / codecov/patch
pyoptsparse/pyNSGA2/pyNSGA2.py#L15
Check warning on line 19 in pyoptsparse/pyNSGA2/pyNSGA2.py
Codecov / codecov/patch
pyoptsparse/pyNSGA2/pyNSGA2.py#L18-L19
Check warning on line 35 in pyoptsparse/pyNSGA2/pyNSGA2.py
Codecov / codecov/patch
pyoptsparse/pyNSGA2/pyNSGA2.py#L35
Check warning on line 16 in pyoptsparse/pyOpt_utils.py
Codecov / codecov/patch
pyoptsparse/pyOpt_utils.py#L12-L16
Check warning on line 579 in pyoptsparse/pyOpt_utils.py
Codecov / codecov/patch
pyoptsparse/pyOpt_utils.py#L579
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related directly to this PR, but is
testflo
raisingImportWarning
by default? I tested just this function locally and the string above (which can be pretty useful) is not printed out by default - see hereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's the default but we can set it explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay that makes sense. I forgot that python tries to import anything that's involved in the init process. I'll approve this now.
Check warning on line 15 in pyoptsparse/pyPSQP/pyPSQP.py
Codecov / codecov/patch
pyoptsparse/pyPSQP/pyPSQP.py#L15
Check warning on line 19 in pyoptsparse/pyPSQP/pyPSQP.py
Codecov / codecov/patch
pyoptsparse/pyPSQP/pyPSQP.py#L18-L19
Check warning on line 34 in pyoptsparse/pyPSQP/pyPSQP.py
Codecov / codecov/patch
pyoptsparse/pyPSQP/pyPSQP.py#L34
Check warning on line 46 in pyoptsparse/pyParOpt/ParOpt.py
Codecov / codecov/patch
pyoptsparse/pyParOpt/ParOpt.py#L46
Check warning on line 16 in pyoptsparse/pySLSQP/pySLSQP.py
Codecov / codecov/patch
pyoptsparse/pySLSQP/pySLSQP.py#L16
Check warning on line 20 in pyoptsparse/pySLSQP/pySLSQP.py
Codecov / codecov/patch
pyoptsparse/pySLSQP/pySLSQP.py#L19-L20
Check warning on line 34 in pyoptsparse/pySLSQP/pySLSQP.py
Codecov / codecov/patch
pyoptsparse/pySLSQP/pySLSQP.py#L34