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

[WIP] Add a command to install CyIpopt #1474

Closed
wants to merge 6 commits into from

Conversation

Robbybp
Copy link
Member

@Robbybp Robbybp commented Aug 19, 2024

Fixes

In conjuction with #1473, addresses part of IDAES/idaes-ext#261. (Not sure if this works on Windows...)

Summary/Motivation:

CyIpopt needs to be able to find a copy of the Ipopt shared library and header files, which it does (at least on Linux and Mac) using pkg-config. To install with Ipopt in a non-standard location (such as IDAES's Ipopt), we need to set the PKG_CONFIG_PATH environment variable (to include .idaes/bin/lib/pkgconfig, which will likely change in new idaes-ext releases). To make this a little bit easier for users, I propose to add a command to install CyIpopt in the current Python environment, e.g.:

idaes install-cyipopt

This basically just runs pip install cyipopt with PKG_CONFIG_PATH set appropriately.

I believe users will still need to set LD_LIBRARY_PATH (or DYLD_LIBRARY_PATH/PATH on Mac/Windows) to actually use CyIpopt. import idaes does set these variables, but I believe import cyipopt uses the value of the variables at the time the Python process started, not the current values in os.environ.

This will only work with idaes-ext 3.4.2

Changes proposed in this PR:

  • Unpack idaes-local* when the get-extensions command is run
  • Add an install-cyipopt command

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

idaes/config.py Outdated
Comment on lines 760 to 774
[oe.get("LD_LIBRARY_PATH", ""), bin_directory]
[
oe.get("LD_LIBRARY_PATH", ""),
bin_directory,
# We add .idaes/bin/lib to LD_LIBRARY_PATH to pick up any libraries
# we introduced by unpacking the idaes-local tar.gz file. This
# directory should be changed when/if the IDAES extensions directory
# structure changes (e.g. to .idaes/lib). -RBP
os.path.join(bin_directory, "lib"),
]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is necessary (or at all beneficial), as the CyIpopt library appears to use the value of the appropriate variable at Python-start-up-time, rather than (somehow) using os.environ.

Comment on lines +405 to 422
tars_to_unpack = list(ptar)
idaes_local_tarname = f"idaes-local-{platform}.tar.gz"
if "darwin" in idaes_local_tarname:
# We neglected to change the name of the idaes-local tar.gz file from *aarch64
# to *arm64 on ARM Mac, so we have to manually patch the name here. I believe
# this is fixed in the idaes-ext main branch, so this can be removed when
# we update to binaries compiled from this branch. -RBP
idaes_local_tarname = idaes_local_tarname.replace("aarch64", "arm64")
idaes_local_tarname = os.path.join(to_path, idaes_local_tarname)
# In addition to the tar.gz files we downloaded, we want to unpack the idaes-local
# tar.gz file. This is included in idaes-solvers, so we can unpack it in the same
# directory after we've unpacked idaes-solvers.
tars_to_unpack.append(idaes_local_tarname)
links = {}
for n, p in zip(pname, ptar):
for p in tars_to_unpack:
_log.debug(f"Extracting files in {p} to {to_path}")
with tarfile.open(p, "r") as f:
_verify_tar_member_targets(f, to_path, links)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we unpack idaes-local* into .idaes/bin, but I think there is no real reason not to unpack it into .idaes, in which case we would end up with bin, lib, include, and share subdirectories of .idaes.

A downside of unpacking idaes-local* is that now we actually distribute two copies of libipopt.so: one in .idaes/bin, and one in idaes/bin/lib... This will be fixed in new versions of idaes-ext.

@codecov-commenter
Copy link

codecov-commenter commented Aug 19, 2024

Codecov Report

Attention: Patch coverage is 16.00000% with 42 lines in your changes missing coverage. Please review.

Project coverage is 76.33%. Comparing base (c2825ca) to head (93aa362).
Report is 15 commits behind head on main.

Files with missing lines Patch % Lines
idaes/commands/extensions.py 12.50% 35 Missing ⚠️
idaes/commands/util/download_bin.py 0.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1474      +/-   ##
==========================================
- Coverage   76.38%   76.33%   -0.06%     
==========================================
  Files         394      394              
  Lines       65121    65167      +46     
  Branches    14427    14436       +9     
==========================================
+ Hits        49745    49747       +2     
- Misses      12813    12858      +45     
+ Partials     2563     2562       -1     

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

@Robbybp Robbybp changed the title Add a command to install CyIpopt [WIP] Add a command to install CyIpopt Aug 22, 2024
@Robbybp
Copy link
Member Author

Robbybp commented Aug 22, 2024

Marking as WIP for now. As it stands, this may not be worth it compared to the alternative of simply documenting all the steps that are necessary to build CyIpopt with IDAES binaries. If we could do everything in a single command, I think it would be worth it. The barrier to this is loading our IPOPT library without relying on a library path from the environment. This may be possible with a patch to CyIpopt? Also, this command should be tested in our CI for Mac, Linux, and Windows.

@ksbeattie ksbeattie requested a review from mrmundt August 22, 2024 18:20
@ksbeattie ksbeattie added the Priority:High High Priority Issue or PR label Aug 22, 2024
@lbianchi-lbl lbianchi-lbl marked this pull request as draft September 30, 2024 19:56
@Robbybp
Copy link
Member Author

Robbybp commented Oct 3, 2024

Closing as I'm not actively working on this. Will re-open if it looks like this will be worth the effort.

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

Successfully merging this pull request may close these issues.

4 participants