-
Notifications
You must be signed in to change notification settings - Fork 11
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
Comments
According to @jsiirola, this is because the path |
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:
with this, I can get CyIpopt working with IDAES-distributed libraries and header files. I also don't have I'm not sure what the right way to distribute a linkable-on-MacOS Ipopt library is. (E.g. make users add |
For what it's worth, the Pyomo-compiled |
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 |
Testing the changes in #262, I think the user will need to set |
@Robbybp same question here - this for an end-of-Nov release? |
@ksbeattie This requires #262 plus and IDAES PR for |
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 |
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. |
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... |
I think pre-release makes more sense.
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.
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.
Just make it very clear that this is a partial, WIP release for integration testing purposes that is not intended for general use. |
@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+. |
Yep just opened a PR to update this (somewhat arbitrarily) to 3.10 #265. |
Will I have to rebuild all the binaries again or is it enough to just re-run the test after #265 is merged? |
You should not need to re-build the binaries |
@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? |
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? |
I just pushed a beta release that includes the ARM binaries for Mac, Ubuntu, and RHEL8. This should be sufficient for testing updates to |
So it seems like GitHub Actions still defaults to Python 3.7, even though we've updated |
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... |
@Robbybp Thoughts on the above comment? |
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. |
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. |
We have an emerging use-case for CyIpopt in WaterTAP. Is there anything I can do to push this along? |
@Robbybp following your advice above, I was able to get
Mostly putting it here so I can return to it later. |
My memory may be a little rusty, but a few months ago, I got 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. |
@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. |
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 runningpython setup.py develop
for CyIpopt: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:When I inspect the CyIpopt library with
otool -l ipopt_wrapper.cpython-39-darwin.so
, I see the following load step: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.
The text was updated successfully, but these errors were encountered: