Skip to content

Conversation

@ewu63
Copy link
Collaborator

@ewu63 ewu63 commented Apr 9, 2025

Purpose

Continuation of #431 but from the mdolab branch to test all optimizers.

Expected time until merged

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

@ewu63 ewu63 requested a review from a team as a code owner April 9, 2025 01:51
@ewu63 ewu63 requested review from ArshSaja and marcomangano April 9, 2025 01:51
@ewu63 ewu63 mentioned this pull request Apr 9, 2025
15 tasks
@codecov
Copy link

codecov bot commented Apr 9, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.74%. Comparing base (19ad2c8) to head (e5cb8a6).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #433      +/-   ##
==========================================
+ Coverage   86.72%   86.74%   +0.01%     
==========================================
  Files          24       24              
  Lines        3435     3439       +4     
==========================================
+ Hits         2979     2983       +4     
  Misses        456      456              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ewu63 ewu63 linked an issue Apr 9, 2025 that may be closed by this pull request
@whophil
Copy link
Contributor

whophil commented Apr 10, 2025

FYI, the meson build currently packages all files in the subdirs of pyoptsparse/. This includes Python source and compiled modules, but ALSO - README's, Fortran and C source, and anything else which might be in those folders. Some of this stuff probably doesn't belong in the built wheel.

There is likely a better way to do this that doesn't include all the extra stuff - perhaps using some sort of file type filter before running install_subdir?

@ewu63 ewu63 mentioned this pull request Apr 13, 2025
13 tasks
@ewu63
Copy link
Collaborator Author

ewu63 commented Apr 13, 2025

Not sure how to do that exactly, the docs aren't very clear. If we can't figure out a way I don't think it's a deal breaker, think it's also been the case since basically the beginning, so I propose that we merge and deal with it in the future.
I did open #434 though and I'd like that merged first, so we can test all the optional installs with mesonpy and make sure that all looks good.

@ewu63 ewu63 removed a link to an issue Apr 14, 2025
@whophil
Copy link
Contributor

whophil commented Apr 14, 2025

@ewu63 this change excludes the source and subdir README files 0dd27fb

@ewu63
Copy link
Collaborator Author

ewu63 commented Apr 15, 2025

@ewu63 this change excludes the source and subdir README files 0dd27fb

Yeah that looks good, feel free to update this PR

@kanekosh
Copy link
Contributor

kanekosh commented Jun 2, 2025

Sorry, I haven't been following this. Is this ready from your side? @ewu63 @whophil

@marcomangano
Copy link
Collaborator

FYI, the real issue is dug into an error log dropdown printout:

  Sanity testing Fortran compiler: flang.exe
  Is cross compiler: False.
  Sanity check compiler command line: flang.exe sanitycheckf.f -o sanitycheckf.exe
  Sanity check compile stdout:
  LINK : fatal error LNK1104: cannot open file 'msvcrt.lib'
  
  -----
  Sanity check compile stderr:
  flang.exe: error: linker command failed with exit code 1104 (use -v to see invocation)

I am not familiar with compilers on Windows, but looking at some discussions like this one (or this one - does the Windows build use Visual Studio in the backend?)we might need to update a compiler option or link to the missing library

@ewu63
Copy link
Collaborator Author

ewu63 commented Aug 14, 2025

I think this is finally ready? CC @marcomangano @eirikurj @whophil @kanekosh

@whophil
Copy link
Contributor

whophil commented Aug 15, 2025

LGTM 🚀

kanekosh
kanekosh previously approved these changes Aug 15, 2025
@marcomangano
Copy link
Collaborator

Maybe it is a problem with my machine, but when I try and editable installation I am getting this error:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<frozen importlib._bootstrap>", line 1360, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1322, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 1262, in _find_spec
  File "$HOME/.pyenv/versions/stable/lib/python3.12/site-packages/_pyoptsparse_editable_loader.py", line 311, in find_spec
    tree = self._rebuild()
           ^^^^^^^^^^^^^^^
  File "$HOME/.pyenv/versions/stable/lib/python3.12/site-packages/_pyoptsparse_editable_loader.py", line 345, in _rebuild
    subprocess.run(self._build_cmd, cwd=self._build_path, env=env, stdout=subprocess.DEVNULL, check=True)
  File "$HOME/.pyenv/versions/3.12.10/lib/python3.12/subprocess.py", line 548, in run
    with Popen(*popenargs, **kwargs) as process:
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "$HOME/.pyenv/versions/3.12.10/lib/python3.12/subprocess.py", line 1026, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "$HOME/.pyenv/versions/3.12.10/lib/python3.12/subprocess.py", line 1955, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: '/tmp/pip-build-env-bglbkx9w/normal/bin/ninja'

It works with the non-editable installation and I double-checked that the issue is introduced with this PR. I am not familiar with Meson machinery, but it looks like it is trying to do some kind of build at runtime and it is missing the ninja library used at build-time and stored in a temporary folder?

@ewu63
Copy link
Collaborator Author

ewu63 commented Aug 15, 2025

Ah I was wondering if I needed to include ninja, pushed. Thanks for being diligent and checking

@marcomangano
Copy link
Collaborator

I am sorry I was unclear, the build works just fine, the error above occurs whenever you try and import pyoptsparse. I am really confused by the fact that the _pyoptsparse_editable_loader.py script generated by meson includes some kind of rebuild on the fly.

@ewu63
Copy link
Collaborator Author

ewu63 commented Aug 15, 2025

I see, it seems that the behaviour here is a little different from setuptools: in an editable install, mesonpy will actually check if the compiled code has been modified, and recompiles as necessarily. This is nice in principle because now the compiled library is also considered "editable", instead of requiring manual deletion of build dir + recompiling. However, it does require the following

  • running pip install -e . with the --no-build-isolation flag, since this secondary compilation step must happen in your dev env -- for some reason mesonpy does not create temporarily build envs for subsequent builds
  • building without isolation means that your dev env must have the build deps installed (which are specified in pyproject.toml). This was the nice thing about setuptools, where it would install build deps for you in a separate build env. This is now done only initially, but in subsequent builds it will have trouble finding the build tools again since that original env has been thrown away.
  • This rebuild nicety is unfortunately not disable-able (see here).

So, I'm not sure what we want to do. I think the dev experience is slightly degraded but not making os.subprocess calls in setup.py seems like a major improvement. I also buy the argument (from meson devs) that editable installs are inherently not isolated builds, so expecting them to be isolated is pointless. We also already require gcc etc. for these installs, so some extra build requirements are not the end of the world if we document these steps properly. What do you all think?

@marcomangano
Copy link
Collaborator

I need to look into the discussion a bit more in detail, but I think a practical solution could be enabling something like pip install -e [dev] . and have the build-time dependencies installed in the same environment "on demand". This would avoid future version headaches with our docker stencil and retain the current editing capabilities. As long as we document this, we should be fine.

Since we only (aside minor exceptions) work with python, it feels weird to recompile (even if it is just through pip) as we do code dev, but I agree that we should just find a work around and move on with ditching setuptools as soon as possible.

@marcomangano
Copy link
Collaborator

EDIT: the [dev] approach might also be a problem because we need numpy >= 2.0 at build time

@ewu63
Copy link
Collaborator Author

ewu63 commented Nov 16, 2025

Circling back on this, what do we think? Is this ready to be merged? @marcomangano @whophil @kanekosh @eirikurj
I would also probably bump the minor version here.

@whophil
Copy link
Contributor

whophil commented Nov 17, 2025

This is a bit old, could somebody sync this with main to see if it still builds?

@marcomangano
Copy link
Collaborator

I will check and test again the PR later (apologies for being AWOL as a maintainer), but I just want to double check if I get the editable build instructions right @ewu63 :

If I don't want to upgrade my whole environment to numpy > 2.0, can I temporarily upgrade numpy, build/install the package, and then revert back to numpy 1.2x.xx to run my optimizations? Or should I just give up on the editable install if I still want to use numpy 1 at runtime?

@marcomangano
Copy link
Collaborator

Regardless, I think this PR warrants a minor version bump instead. Given the conversation in #459 and the doc update in #466 (

* Python 3.10+
) we should probably bump the minimum python version in pyproject.toml and environment.yml

@ewu63
Copy link
Collaborator Author

ewu63 commented Nov 19, 2025

If I don't want to upgrade my whole environment to numpy > 2.0, can I temporarily upgrade numpy, build/install the package, and then revert back to numpy 1.2x.xx to run my optimizations? Or should I just give up on the editable install if I still want to use numpy 1 at runtime?

I think that should work but not 100% sure. In general I caution against editable installs unless you are actually developing stuff in pyOptSparse.

I found a quirky issue with editable installs. Because meson will use a separate build dir to store the compiled .so libraries (i.e. out-of-tree build) and rely on some import hook to manage this, our import_module function did not work as it expects the library to be exactly where we put it. I pushed a little hack which worked for me but is definitely fragile, we can probably improve it by either doing some relative path calculations, or more explicit in all the conditions we want to try the import:

  1. import slsqp from this exact path (either the actual path for in-source builds or special SNOPT path var)
  2. import pyoptsparse.pySLSQP.slsqp from sys.path <- this is what we need with editable installs + mesonpy

Thoughts @whophil ?

EDIT: the [dev] approach might also be a problem because we need numpy >= 2.0 at build time
Not necessarily. It is preferred but if you just stick to numpy1 on your local machine then it will still work. The binaries will not work if you subsequently install numpy2 but you should not expect that to work. Any compat issue with numpy1/2 will be caught by CI so I'm not too worried.

I am OK with adding a separate group of optional deps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants