search

sigma-toolkit / moab-python

GNU Lesser General Public License v2.1
1 stars 0 forks source link

MOAB Build Configuration - Dependency Inclusion for Wheel #1

Open ahnaf-tahmid-chowdhury opened 2 months ago

ahnaf-tahmid-chowdhury commented 2 months ago

I have successfully built the MOAB wheels with HDF5 support enabled for PyMOAB. However, I am facing some decisions regarding which dependencies to include for optimal functionality and size reduction.

Current Setup:

Default Configuration:

Proposed Changes:

  1. Enable Eigen: Turning on Eigen while disabling gfortran and LAPACK could reduce the wheel size by 60%.
  2. Disable gfortran and LAPACK: These libraries are currently enabled but may not be necessary, especially if Eigen is used.

Request for Input:

You can find the successful builds here.

Tagging: @vijaysm, @gonuke, @pshriwise

vijaysm commented 2 months ago

The reasoning for including LAPACK by default is because most machines have some flavor of BLAS/LAPACK available and if not, it's easy for admin to do so. Eigen3 is almost never available and an admin/user always has to install it. This was more about the ease of dependency.

I am refactoring Matrix3 to eliminate some of these hard constraints. Will submit a PR for review this week.

vijaysm commented 2 months ago

Until my mpi4py branch works with latest master, we may not need any other additional dependency. I was going to suggest Netcdf but in a serial build, not sure how much value that adds.

gonuke commented 2 months ago

The reasoning for including LAPACK by default is because most machines have some flavor of BLAS/LAPACK available and if not, it's easy for admin to do so. Eigen3 is almost never available and an admin/user always has to install it. This was more about the ease of dependency.

I am refactoring Matrix3 to eliminate some of these hard constraints. Will submit a PR for review this week.

But Eigen3 is a header only library and doesn't need an admin to install.

LAPACK requires FORTRAN, and can be the only reason for FORTRAN, thus adding a constraint.

gonuke commented 2 months ago

I recommend reviewing the conda-forge build configurations, since we discussed some of these things there a few years ago.

ahnaf-tahmid-chowdhury commented 2 months ago

Here is the related issue in the conda-forge: https://github.com/conda-forge/moab-feedstock/issues/27

shimwell commented 2 months ago

Link to the dependencies used in conda moab https://github.com/conda-forge/moab-feedstock/blob/cbcb576742e51eebbb0b73acf5f8b289048ed62b/recipe/meta.yaml#L60-L92

gonuke commented 2 months ago

Here is the related issue in the conda-forge: https://github.com/conda-forge/moab-feedstock/issues/27

I think that's out of date, though, since we do depend on LAPACK in the current build

gonuke commented 2 months ago

They became dependencies again 3 years ago: https://github.com/conda-forge/moab-feedstock/pull/50

Many years ago, removing BLAS/LAPACK dependency was valuable for building on Windows with native compilers. (i.e. no FORTRAN!). I was definitely pushing for it then for local user use cases other than shared HPC use cases.

ahnaf-tahmid-chowdhury commented 2 months ago

Upon reviewing the build.sh script, it seems Eigen is enabled while Fortran is disabled.

ahnaf-tahmid-chowdhury commented 2 months ago

It seems MOAB two versions: a general version and one with MPI support. Therefore, I think, it might be beneficial to implement a similar approach with pip.

ahnaf-tahmid-chowdhury commented 1 month ago

To address the issue of multiple variants, I created an issue on scikit-build-core to explore potential solutions. Based on the feedback, it seems we may need to create separate packages for each variant. Please advise on the next steps for proceeding with this approach.

vijaysm commented 1 month ago

The PR https://bitbucket.org/fathomteam/moab/pull-requests/700 should fix all of these issues. You don't need Eigen3 or LAPACK now as a requirement. The only reason why any of these were a requirement was due to the eigen decomposition code. Now, I have a native implementation that works as well as LAPACK for cases tested. Review the PR and comment there.

vijaysm commented 1 month ago

LAPACK requires FORTRAN, and can be the only reason for FORTRAN, thus adding a constraint.

@gonuke Not really. I removed this requirement ages ago. You can specify the mangling format directly and it will be used without needing Fortran anywhere. At least, I have this working and used with autotools workflows. Can't remember if I did the same in CMake previously.

gonuke commented 1 month ago

Good to hear @vijaysm - I guess my motivations are outdated.