-
Notifications
You must be signed in to change notification settings - Fork 119
Improve conditional imports #458
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
Improve conditional imports #458
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #458 +/- ##
===========================================
- Coverage 86.22% 74.97% -11.25%
===========================================
Files 24 24
Lines 3419 3429 +10
===========================================
- Hits 2948 2571 -377
- Misses 471 858 +387 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
kanekosh
left a comment
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.
Thanks! Just a minor comment from me.
Linting and formatting checks are failing because we updated the Azure setup to switch to ruff and pre-commit, but we haven't merged the code update PR #455 yet. So please don't worry about it.
tests/test_other.py
Outdated
| def test_sys_path_unchanged(self): | ||
| path = tuple(sys.path) | ||
| try_import_compiled_module_from_path("snopt", "/some/path") | ||
| import_module("snopt", "/some/path") |
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.
I believe this should be import_module("snopt", ["/some/path"]), otherwise import_module loops over each letter of /some/path.
Maybe it'd be nice to add an explicit check on the path type in the import_module function?
ewu63
left a comment
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.
Left minor comments, looks good to me.
| THIS_DIR = os.path.dirname(os.path.abspath(__file__)) | ||
| _IMPORT_SNOPT_FROM = os.environ.get("PYOPTSPARSE_IMPORT_SNOPT_FROM", THIS_DIR) | ||
| snopt = try_import_compiled_module_from_path("snopt", _IMPORT_SNOPT_FROM) | ||
| _IMPORT_SNOPT_FROM = os.environ.get("PYOPTSPARSE_IMPORT_SNOPT_FROM", None) or THIS_DIR |
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 to handle an empty str set to the var?
|
This PR isn't testing all the optimizers since it came from a public fork, can someone in the lab test this out? Maybe @A-CGray ? |
Tested this on our latest private image with all optimisers, everything passes |
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.
Looks good to me, thanks Phil! Just looks like ruff wanted an extra line in one file.
Purpose
This PR improves the conditional imports.
sys.pathbefore attempting import, this prepends tosys.pathsys.path. I regret suggestingPYOPTSPARSE_IMPORT_SNOPT_FROM, but we can keep it for now.try_import_compiled_module_fromare returned so that they can be re-raised later with a meaningful traceback.Expected time until merged
1 week
Type of change
This is technically breaking if you considered
pyOpt_utils.try_import_module_from_pathto be part of the public API.Testing
Checklist
ruff checkandruff formatto make sure the Python code adheres to PEP-8 and is consistently formattedfprettifyor C/C++ code withclang-formatas applicable