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

Refactor Python bindings #151

Closed
wants to merge 78 commits into from

Conversation

stubbiali
Copy link
Contributor

@stubbiali stubbiali commented Oct 24, 2023

Profound restructuring and refining of the Python bindings. Since this effort is driven by the needs of the FVM model, the focus has primarily been on structured grids so far.

  • The Python and C++ files are kept in separate directories, respectively bindings/python/src/ghex and bindings/python/src/_pyghex;
  • The Python package is renamed from pyghex to ghex, and all package-level configurations are dumped into pyproject.toml;
  • Split Python code into modules to reflect the file structure of C++ source directory;
  • Switch build backend from scikit-build-core to setuptools.build_meta; this is mainly motivated by the flexibility provided by the latter, which allows to customize the build process using environment variables and so allows to e.g. enable GPU support and specify the transport backend;
  • Fix check on the memory layout of the buffer to exchange;
  • Add support for HIP/ROCm by supporting the __hip_array_interface__ (introduced by Python: Add support for HIP/ROCm buffers GridTools/gridtools#1759 and feature[cartesian]: Add support for HIP/ROCm GridTools/gt4py#1278);
  • Fix compilation on NVIDIA GPUs;
  • Adapt unit tests;
  • Apply the black formatter to all Python source files and add annotations;
  • Minor bug-fix in clang-format config file.

The code has been tested on Mac (M2 processor), Piz Daint and LUMI.

@boeschf
Copy link
Collaborator

boeschf commented Nov 8, 2023

Hi @stubbiali ! Thanks for all the work - looks good! I'll try to give you extended feedback today or tomorrow. Could you maybe merge with master and resolve conflicts?

@boeschf
Copy link
Collaborator

boeschf commented Nov 16, 2023

allows to customize the build process using environment variables and so allows to e.g. enable GPU support and specify the transport backend;

Is there any other/additional way to customize the build process? Environment variables are fine, but maybe command line arguments would be more ergonomic/more explicit.

@boeschf
Copy link
Collaborator

boeschf commented Nov 16, 2023

A small thing that rubs me the wrong way is that the build directory is created in the source tree next to setup.py (build_dir = os.path.abspath(os.path.join(this_dir, "build"))) - do you think we can change this? Not sure where it should be, maybe in tmp or somewhere else (or be configurable)?

@stubbiali
Copy link
Contributor Author

Is there any other/additional way to customize the build process? Environment variables are fine, but maybe command line arguments would be more ergonomic/more explicit.

I tend to agree that command line arguments would be more elegant. The only way I know to accomplish this would be through config settings. To be honest, I've never used this feature, but to me it doesn't look like it makes the customization of the build process either more explicit or easier to read. So I would stick to environment variables, which is a more common approach.

@stubbiali
Copy link
Contributor Author

A small thing that rubs me the wrong way is that the build directory is created in the source tree next to setup.py (build_dir = os.path.abspath(os.path.join(this_dir, "build"))) - do you think we can change this? Not sure where it should be, maybe in tmp or somewhere else (or be configurable)?

We could use a temporary folder as the build directory with build_dir = tempfile.mkdtemp(). Configuration via env variable is also possible, e.g. the build directory could be created under GHEX_BUILD_ROOT, rather than in the system-wide temporary directory.

@boeschf boeschf mentioned this pull request Jan 25, 2024
@boeschf
Copy link
Collaborator

boeschf commented Mar 20, 2024

closing this as it is superseded by #156

@boeschf boeschf closed this Mar 20, 2024
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.

2 participants