Skip to content

Conversation

@mlee03
Copy link
Contributor

@mlee03 mlee03 commented May 29, 2025

Description
With the current setup, pip install will not distribute the compiled cFMS library when installing pyFMS. The following changes have been made to fix the installation issue:

  1. FMS and cFMS compilation is no longer part of pyFMS pip installation. Instead, the users will need to compile
    the libraries manually by using the compile.py script. This way, pip installing pyFMS will be less time consuming and compiling cFMS/FMS will be more foolproof. The libraries will be installed to pyfms/lib

  2. Adds a MANIFEST.in to include the compiled libraries as part of pip install.

  3. Adds a README for installation directions

@mlee03 mlee03 requested review from fmalatino and rem1776 May 29, 2025 18:02
@mlee03
Copy link
Contributor Author

mlee03 commented May 29, 2025

The --oversubscribe specification breaks run_tests.sh with MPICH. This script was only a temporary fix to the broken CI. Testing should be more streamlined to not use this script if possible.

@fmalatino fmalatino requested a review from bensonr May 30, 2025 11:25
Copy link
Contributor

@fmalatino fmalatino left a comment

Choose a reason for hiding this comment

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

PR 7 also introduced a MANFEST.in file and changed the setup.py file to override the build command, enabling the compilation and inclusion of the shared object from cFMS from a single call to pip install. Could this PR be changed such that subprocesses called during the override of build will check for the existence of the shared object file, whether it is compiled from a totally separate method, or from the compile.py file, otherwise it will use the compile.py file to generate it? This will enable the package to still be completely installed via a single pip install.

@bensonr
Copy link

bensonr commented May 30, 2025

@mlee03 - I agree with @fmalatino that we need the most flexible installation processes possible. For Pace work, we want to ensure a user doesn't need a manual multi-stage installation process that may not be well understood.

Copy link

@bensonr bensonr left a comment

Choose a reason for hiding this comment

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

With your suggested change to provide pre-compiled versions of FMS and cFMS by default, you will need to remove the submoduled in cFMS.

Comment on lines +5 to +9
# path to the installed yaml library
yaml = "/opt/libyaml/0.2.5/GNU/14.2.0/"

# path to the installed netcdf library
netcdf = "/opt/netcdf/4.9.3/GNU/14.2.0/"
Copy link

Choose a reason for hiding this comment

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

Not ideal as they are hard-coded for a specific versions and a specific machine. I know this is addressed in the Readme.

Comment on lines +37 to +38
FC = "mpif90"
CC = "mpicc"
Copy link

Choose a reason for hiding this comment

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

This requires an MPI library be accessible. You'll need to mention that as well. Is there any chance this could become an issue if mpi4py and the MPI used here are different flavors?

@mlee03
Copy link
Contributor Author

mlee03 commented Jun 2, 2025

I'll add back in the compile and installation of FMS and cFMS during pip install. However, the current method is not reliable. Reading up a bit hints that we should go with a meson build system for cFMS. Building cFMS is much simpler than FMS because there is no inter-module dependencies in cFMS, so I don't think it will be that difficult to set up the build without using autotools and cmake. And packages like numpy and scipy that have compiled backend libraries use meson. I'll change this PR into a draft.

@mlee03 mlee03 marked this pull request as draft June 2, 2025 19:09
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.

3 participants