Skip to content
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

Distribute Ipopt library and headers that can be used with CyIpopt #261

Open
Robbybp opened this issue Sep 11, 2023 · 30 comments
Open

Distribute Ipopt library and headers that can be used with CyIpopt #261

Robbybp opened this issue Sep 11, 2023 · 30 comments
Assignees
Labels
Priority:Normal Normal Priority Issue or PR WaterTAP

Comments

@Robbybp
Copy link
Member

Robbybp commented Sep 11, 2023

I'd like to use CyIpopt because of its potential to help with debugging, and it would be nice to use it with IDAES-provided binaries. CyIpopt needs an Ipopt shared library and Ipopt include files. The Ipopt shared library that we distribute should work, and it shouldn't be too hard to distribute the include files in an .idaes/include directory. I can patch CyIpopt to use our Ipopt library, which gives me the following link step (on MacOS) when running python setup.py develop for CyIpopt:

clang -bundle -undefined dynamic_lookup -arch arm64 -arch x86_64 -Wl,-headerpad,0x1000 build/temp.macosx-10.9-universal2-3.9/cyipopt/cython/ipopt_wrapper.o -L/Users/robert/.idaes/bin -lipopt -o /Users/robert/cyipopt/ipopt_wrapper.cpython-39-darwin.so

This seems to successfully compile the ipopt_wrapper.cpython-39-darwin.so library that CyIpopt uses, but trying to use this (e.g. run the CyIpopt tests) gives me the following run-time error:

E   ImportError: dlopen(/Users/robert/cyipopt/ipopt_wrapper.cpython-39-darwin.so, 0x0002): Library not loaded: /Users/john/src/idaes-ext-build/coinbrew/dist-share/lib/libipopt.3.dylib
E     Referenced from: <3E860C76-3127-3257-A4B6-8841E9BD66A3> /Users/robert/cyipopt/ipopt_wrapper.cpython-39-darwin.so
E     Reason: tried: '/usr/local/lib/libipopt.3.dylib' (no such file), '/libipopt.3.dylib' (no such file), '/Users/robert/.pyomo/lib/libipopt.3.dylib' (no such file), '/Users/john/src/idaes-ext-build/coinbrew/dist-share/lib/libipopt.3.dylib' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/Users/john/src/idaes-ext-build/coinbrew/dist-share/lib/libipopt.3.dylib' (no such file), '/Users/john/src/idaes-ext-build/coinbrew/dist-share/lib/libipopt.3.dylib' (no such file), '/usr/local/lib/libipopt.3.dylib' (no such file), '/usr/lib/libipopt.3.dylib' (no such file, not in dyld cache)

When I inspect the CyIpopt library with otool -l ipopt_wrapper.cpython-39-darwin.so, I see the following load step:

Load command 10
          cmd LC_LOAD_DYLIB
      cmdsize 104
         name /Users/john/src/idaes-ext-build/coinbrew/dist-share/lib/libipopt.3.dylib (offset 24)
   time stamp 2 Wed Dec 31 17:00:02 1969
      current version 17.2.0
compatibility version 17.0.0

I am not entirely sure why CyIpopt is trying to follow this path for linking. Could this be due to a missing -fPIC flag somewhere? I have not been able to reproduce this behavior compiling Ipopt locally (even when I move the compiled Ipopt files to a new location before linking).

I will continue to investigate, but wanted to post here in case this rings a bell for anyone and so I can reference it later.

@ksbeattie ksbeattie added the Priority:Normal Normal Priority Issue or PR label Sep 14, 2023
@Robbybp
Copy link
Member Author

Robbybp commented Sep 19, 2023

According to @jsiirola, this is because the path /Users/john/src/idaes-ext-build/coinbrew/dist-share/lib/libipopt.3.dylib is included in the pkg-config file we distribute. I'm not sure how this is getting picked up in the compilation above, because IIRC I'm just linking the libipopt.dylib file that we distribute and bypassing pkg-config (I get the include files directly from a local Ipopt install). I'll double check when I'm back on that computer.

@Robbybp
Copy link
Member Author

Robbybp commented Sep 30, 2023

The problem I encountered seems to occur because the "install name" of the library we distribute on MacOS is set to some path on John Eslick's machine/VM. Luckily, we can change this pretty easily:

install_name_tool -id $HOME/.idaes/lib/libipopt.3.dylib $HOME/.idaes/lib/libipopt.dylib
install_name_tool -id $HOME/.idaes/lib/libipopt.3.dylib $HOME/.idaes/lib/libipopt.3.dylib

with this, I can get CyIpopt working with IDAES-distributed libraries and header files.

I also don't have $HOME/.idaes/lib on my LD_LIBRARY_PATH or DYLD_LIBRARY_PATH. I could probably have fixed this just by adding this to the dyld path.

I'm not sure what the right way to distribute a linkable-on-MacOS Ipopt library is. (E.g. make users add $HOME/.idaes/lib to their DYLD_LIBRARY_PATH or run install_name_tool on these libraries as part of idaes get-extensions.) It seems like the .dylib file format has some tokens (e.g. @rpath) that might be useful? (See https://github.com/qyang-nj/llios/blob/main/macho_parser/docs/LC_dylib.md)

@Robbybp
Copy link
Member Author

Robbybp commented Sep 30, 2023

For what it's worth, the Pyomo-compiled libpynumero_MA27.dylib library has an "install name" that is @rpath/libpynumero_MA27.dylib. I don't think we set this explicitly (via ld -install_name @rpath/<libname>) for either the Pyomo or IDAES libraries, so I'm not sure why they are being set differently. (Could be a missing -fPIC flag?)

@Robbybp
Copy link
Member Author

Robbybp commented Sep 30, 2023

Accomplishing this will require modifications to two repos:

  • Update idaes get-extensions (or possibly some other option of the idaes command) to unpack the idaes-lib-<os>.tar.gz and idaes-local-<os>.tar.gz files. This seems to happen here, although I'm not sure exactly what will have to change at this point.
  • Update the distributed extensions (specifically, libipopt and ipopt.pc) to not reference specific paths on John E.'s machine. The libipopt modification may only be relevant on MacOS. This happens in this repo, but again I'm not entirely sure what will need to change.

Then, the user-facing instructions to install CyIpopt with our IDAES binaries will be:

pip install idaes
idaes get-extensions
export PKG_CONFIG_PATH=$PKG_CONFIG_PATH:$HOME/.idaes/lib/pkgconfig
pip install cyipopt

(They may also need to set LD_LIBRARY_PATH to include .idaes/lib in order to find the library at run-time. This isn't necessary on MacOS with -install_name @rpath/<libname> during compilation, but I haven't tested on Linux or Windows.)

@Robbybp
Copy link
Member Author

Robbybp commented Oct 3, 2023

Testing the changes in #262, I think the user will need to set DYLD_LIBRARY_PATH on MacOS in order for the @rpath install names to work (i.e. for CyIpopt to find libipopt.dylib at run-time).

@ksbeattie
Copy link
Member

@Robbybp same question here - this for an end-of-Nov release?

@Robbybp
Copy link
Member Author

Robbybp commented Nov 16, 2023

@ksbeattie This requires #262 plus and IDAES PR for idaes command updates. It will probably be completed one week behind #262, if no issues with the command updates pop up.

@Robbybp
Copy link
Member Author

Robbybp commented Dec 19, 2023

Changes to make the libraries linkable have been merged in #262. @bpaul4 @MarcusHolly Can you work on a release of the Windows and Linux builds?

@MarcusHolly
Copy link
Contributor

Changes to make the libraries linkable have been merged in #262. @bpaul4 @MarcusHolly Can you work on a release of the Windows and Linux builds?

So we just need to generate the tar files, right? Just as a sanity check, I think I should remake the ones I've generated (from #262) a few weeks ago. I should be able to have this done by some point tomorrow

@Robbybp
Copy link
Member Author

Robbybp commented Dec 19, 2023

Yes, this should be as easy as uploading the tar files from the build process to the releases page. Yes, I would recommend you remake the tar files for sanity. Thanks! Let me know if you run into any unexpected issues.

@MarcusHolly
Copy link
Contributor

@Robbybp

I was able to generate the tar files without any issues for the Windows and Linux builds. I followed the naming and tag conventions for pre-releases in the IDAES repo, but I'm not sure what the description should be for the release.

A few other questions/comments:

1.) Should I publish it as a pre-release or a regular release?

2.) I noticed previous releases in idaes-ext include tar files for darwin. What are these? The Mac files?

3.) Lastly, I've just realized that I only generated the tar files for x86_64. I tried generating the aarch64 files, but I don't know if it'll even work on my laptop...

@MarcusHolly
Copy link
Contributor

This is the error I get when trying to run aarch64
image

@Robbybp
Copy link
Member Author

Robbybp commented Dec 20, 2023

1.) Should I publish it as a pre-release or a regular release?

I think pre-release makes more sense.

2.) I noticed previous releases in idaes-ext include tar files for darwin. What are these? The Mac files?

Yes. These are for Mac. You'll need a Mac to compile these. My current plan is that I will compile these separately (without HSL), which we can use for testing until you or Brandon have access to a Mac.

3.) Lastly, I've just realized that I only generated the tar files for x86_64. I tried generating the aarch64 files, but I don't know if it'll even work on my laptop...

I believe this is expected. Our docker images are not set up to build cross-architecture. I'm not sure why the docker build scripts accept the architecture as an argument. You will need an ARM machine to build the aarch64 files. I believe this is typically done from an apple-silicon Mac. Assuming you don't have access to an ARM machine, I can build these binaries without HSL.

My recommendation is to make a "partial" pre-release with just the files you can build, then I will send you the binaries I am responsible for (all ARM builds without HSL), which you can include in a follow-up pre-release.

but I'm not sure what the description should be for the release

Just make it very clear that this is a partial, WIP release for integration testing purposes that is not intended for general use.

@MarcusHolly
Copy link
Contributor

@Robbybp I ran the Windows test via GitHub actions on the 3.5.0beta pre-release tag: https://github.com/IDAES/idaes-ext/actions/runs/7279400306. It failed pretty quickly...

@lbianchi-lbl
Copy link

@Robbybp I ran the Windows test via GitHub actions on the 3.5.0beta pre-release tag: https://github.com/IDAES/idaes-ext/actions/runs/7279400306. It failed pretty quickly...

I believe the Python version being used is too old. IDAES requires Python 3.8+.

@Robbybp
Copy link
Member Author

Robbybp commented Dec 20, 2023

Yep just opened a PR to update this (somewhat arbitrarily) to 3.10 #265.

@MarcusHolly
Copy link
Contributor

Will I have to rebuild all the binaries again or is it enough to just re-run the test after #265 is merged?

@Robbybp
Copy link
Member Author

Robbybp commented Dec 20, 2023

You should not need to re-build the binaries

@MarcusHolly
Copy link
Contributor

@Robbybp I've merged in the Python update PR, but the same failure occurs with node12 being deprecated. I'm assuming Python 3.10 is supposed to run on node16, so maybe I do need to rebuild the binaries?

@Robbybp
Copy link
Member Author

Robbybp commented Dec 22, 2023

With what I'm seeing here, it looks like we were still trying to set up Python 3.7. Not sure what the reason could be. Or are you referring to a different failure?

@Robbybp
Copy link
Member Author

Robbybp commented Dec 22, 2023

I just pushed a beta release that includes the ARM binaries for Mac, Ubuntu, and RHEL8. This should be sufficient for testing updates to idaes get-extensions, and for testing CyIpopt against these libraries. However, for the final 3.5.0 release, we will want binaries for all platforms compiled and tested with HSL.

@MarcusHolly
Copy link
Contributor

So it seems like GitHub Actions still defaults to Python 3.7, even though we've updated test_windows_main.yml. I'm not quite sure how to get around this issue.

@MarcusHolly
Copy link
Contributor

Small update on the GitHub actions front:

I ended up re-making the binaries, which addressed Python version issue from the comment above. Currently, the windows build is passing in GitHub actions but there are a few errors in the Linux builds. I'm not sure how those should be addressed...

@MarcusHolly
Copy link
Contributor

@Robbybp Thoughts on the above comment?

@Robbybp
Copy link
Member Author

Robbybp commented Jan 4, 2024

I haven't had time to look at the errors. I hit some segfaults testing our Mac builds with CyIpopt and Mumps that I've been trying to isolate. I hope to get to the GHA Linux errors early next week.

@Robbybp
Copy link
Member Author

Robbybp commented Jan 4, 2024

Looking briefly, it is only two test failures on a tray column solve. TBH I am more encouraged that the rest of the tests pass, given that it is our first time going through this build process (although these failures should still be addressed). Looking at the last linux tests 7 months ago, https://github.com/IDAES/idaes-ext/actions/runs/5212024395, the currently failing platforms (rocky8, ubuntu18, and debian10) all passed, although a few other OSs failed at that time. Unfortunately, the old test logs have expired. I will prioritize testing the Mac build with Mumps for now, and we can discuss the test failures with the group next Thursday.

@bknueven
Copy link

bknueven commented Jun 9, 2024

We have an emerging use-case for CyIpopt in WaterTAP. Is there anything I can do to push this along?

@bknueven
Copy link

bknueven commented Jun 10, 2024

@Robbybp following your advice above, I was able to get cyipopt to compile against the IDAES distribution by manually downloading the package, setting PKG_CONFIG_PATH as needed, and making the below modification to the ipopt.pc file.

15d14
< Requires.private: coinhsl coinmumps 

Mostly putting it here so I can return to it later.

@adowling2
Copy link
Contributor

My memory may be a little rusty, but a few months ago, I got cyipopt to install via conda. After installing cyipopt, I also installed the HSL libraries for macOS-arm. (I have an academic license.)

However, I think had a seg fault issue, which may have been due to something else. I then switched to a Linux machine: CCSI-Toolset/measurement-optimization#2

I'm sharing these details here while I remember them. Sorry, theses may not be that helpful.

@ksbeattie
Copy link
Member

We have an emerging use-case for CyIpopt in WaterTAP. Is there anything I can do to push this along?

@bknueven I've placed this on the Aug release board to raise awareness of it during dev calls. Currently though we don't have a clearly identified and dedicated owner of the idaes-ext builds. Hopefully that will change soon, with perhaps someone at Sandia taking it over.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:Normal Normal Priority Issue or PR WaterTAP
Projects
None yet
Development

No branches or pull requests

6 participants