search

IDAES / idaes-ext

IDAES developer repo for those building the idaes binary solvers and related tools.
Other
9 stars 11 forks source link

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

Open Robbybp opened 1 year ago

Robbybp commented 1 year ago

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.

Robbybp commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Accomplishing this will require modifications to two repos:

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 commented 1 year ago

Testing the changes in https://github.com/IDAES/idaes-ext/pull/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 commented 1 year ago

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

Robbybp commented 1 year ago

@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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

@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 commented 11 months ago

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

Robbybp commented 11 months ago

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 commented 11 months ago

@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 commented 11 months ago

@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 commented 11 months ago

Yep just opened a PR to update this (somewhat arbitrarily) to 3.10 https://github.com/IDAES/idaes-ext/pull/265.

MarcusHolly commented 11 months ago

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

Robbybp commented 11 months ago

You should not need to re-build the binaries

MarcusHolly commented 11 months ago

@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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

@Robbybp Thoughts on the above comment?

Robbybp commented 10 months ago

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 commented 10 months ago

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 commented 5 months ago

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

bknueven commented 5 months ago

@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 commented 5 months ago

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: https://github.com/dowlinglab/measurement-opt/issues/2

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

ksbeattie commented 5 months ago

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.