Add ability to specify search path for SNOPT import with env var

[whophil]

Purpose

Addresses #337

I am totally open to suggestions on the name of this environment variable. For now it is explicit as possible.

Expected time until merged

Quickly - due to small scope of change, not urgency.

Type of change

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (non-backwards-compatible fix or feature)
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no API changes)
• Documentation update
• Maintenance update
• Other (please describe)

Testing

Checklist

• I have run flake8 and black to make sure the Python code adheres to PEP-8 and is consistently formatted
• I have formatted the Fortran code with fprettify or C/C++ code with clang-format as applicable
• I have run unit and regression tests which pass locally with my changes
• I have added new tests that prove my fix is effective or that my feature works
• I have added necessary documentation

[codecov]

Codecov Report

Merging #338 (b408fb4) into main (98a7910) will decrease coverage by 11.53%.
The diff coverage is 95.00%.

@@             Coverage Diff             @@
##             main     #338       +/-   ##
===========================================
- Coverage   83.92%   72.40%   -11.53%     
===========================================
  Files          22       22               
  Lines        3347     3363       +16     
===========================================
- Hits         2809     2435      -374     
- Misses        538      928      +390     

Impacted Files	Coverage Δ
pyoptsparse/pySNOPT/pySNOPT.py	16.61% <95.00%> (-73.30%)	⬇️

... and 7 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[marcomangano]

Thanks for the PR Phil! I just have two quick comments:

• I like using a self-explanatory name for the environmental variable, even if long. I am just not convinced about the _NONRELATIVE part, it might not be intuitive for users. What about something along the lines of PYOPTSPARSE_LOAD_COMPILED_SNOPT? Would that be ambiguous? I have no strong opinions on this name myself .
• Could you update the docs here? Just clarifying the scenario in which this approach is recommended (as you did in Allow snopt module to be loaded non-relatively #337) and a snippet with the updated env variables is enough imho

[whophil]

@marcomangano I pushed a change renaming the variable to PYOPTSPARSE_IMPORT_SNOPT_ABSOLUTE, and also added info to docs.

[whophil]

The failing test seems to be related to NSGA segfaults (#340), otherwise this is ready.

[whophil]

A clarification on how this behaves -

The original search order for snopt was:

1. The directory relative to pySNOPT.py

With this change, if you specify a path with PYOPTSPARSE_SNOPT_PATH_PREPEND, the search order for snopt becomes:

1. The path specified by PYOPTSPARSE_SNOPT_PATH_PREPEND
2. Standard PYTHONPATH
3. The directory relative to pySNOPT.py

The second item may be unexpected, so I wanted to point it out.

[marcomangano]

Thanks for the update. Aside the comment below, I feel your note on the search order should be included in the documentation.

I see how searching through the "original" PYTHONPATH is inevitable with this approach. I don't have anything against it, but I think we need some kind of note and/or warning. I can see how more or less intentionally a user could set an invalid env variable but still end up importing any snopt library already existing on the path. I don't think the logic should change though. Do you have other thoughts @eirikurj

[whophil]

@marcomangano there is one other option, which has no hidden behavior. I will explain it in words and wait for your feedback before implementing.

1. If the env var PYOPTSPARSE_LOAD_SNOPT_FROM exists, prepend PYTHONPATH with that path and import snopt.
2. Check where snopt was imported from.
3. If it wasn't imported from the specified path, don't use it! Set snopt = None

There is also the option to not attempt the relative import as a fallback if the absolute import fails. This feels a little more intuitive to me.

Personally, I would prefer an error to be raised if PYOPTSPARSE_LOAD_SNOPT_FROM is specified and snopt cannot be successfully loaded from that path, but that seems to be out of line with the snopt = None approach.

[marcomangano]

1. If the env var `PYOPTSPARSE_LOAD_SNOPT_FROM` exists, prepend `PYTHONPATH` with that path and `import snopt`.

2. Check where `snopt` was imported from.

3. If it wasn't imported from the specified path, don't use it! Set `snopt = None`

I like this approach, it avoids the ambiguous scenarios I mentioned above.

There is also the option to not attempt the relative import as a fallback if the absolute import fails. This feels a little more intuitive to me.

I also agree with this. pyOptSparse is either importing the library specified in the path or dropping the import, no ambiguous fallbacks.

Personally, I would prefer an error to be raised if PYOPTSPARSE_LOAD_SNOPT_FROM is specified and snopt cannot be successfully loaded from that path, but that seems to be out of line with the snopt = None approach.

I think this might be redundant, as we indeed have a check on the correct import here. Maybe we can print a warning for this scenario, but if we end up implementing your latest suggestion I think there is no need for an additional error imho.

[marcomangano]

This looks good to me! Just fixed a minor inconsistency in the docs. Maybe @eirikurj has some final comments on this?

[whophil]

This looks good to me! Just fixed a minor inconsistency in the docs. Maybe @eirikurj has some final comments on this?

Thanks @marcomangano !

I just pushed one other basic test for _import_snopt_from_path. It doesn't complete the test coverage, but it's better than nothing.

[eirikurj]

Thanks for your effort on this and the tests. I just have few very minor questions/comments, otherwise it looks good.

[eirikurj]

Changes look good. Not sure, but the U22 image failed a couple of times installing the build deps. The current failure is the #340 issue, which is unrelated, so I am approving.