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

Compiled backends integration #835

Merged
merged 89 commits into from
Jul 22, 2022
Merged

Conversation

petiaccja
Copy link
Contributor

@petiaccja petiaccja commented Jun 24, 2022

Description

Introduces an organized way to take source code generated from iterator IR, generate Python bindings for it, compile the source and the bindings into a Python module, and load it back into Python.

Operation:

  1. ITIR -> SourceCodeModule: code generators generate a source code module from iterator IR. The module contains the source (i.e. C++) representation of the ITIR plus the required dependencies (i.e. GridTools, CUDA) and the signature of the entry point (i.e. fencil)
  2. SourceCodeModule -> BindingModule: the binding module uses the entry point to generate the code (in the same language) for the Python bindings. Also contains the dependencies needed by the bindings (i.e. pybind11).
  3. SourceCodeModule + BindingModule -> dynamic library: the source codes and dependencies are organized on the file system and compiled into a binary. (For example, the CMakeLists.txt is generated, file are written to disk, and compiled using CMake.)
  4. dynamic library -> Callable: the compiled python module is loaded and the functions inside it can be used to call the compiled stencil.

Test parametrization:
Integration tests are parametrized to run with multiple backends, including compiled and immediate.

Known issues:

  • GTFN C++ bindings cannot do unstructured
  • GTFN C++ dimensions are not assigned proper names

TODO:

Requirements

Before submitting this PR, please make sure:

  • You have run the code checks, tests and documentation build successfully
  • All fixes and all new functionality are tested and documentation is up to date
  • You looked at the review checklist

Additionally, if this PR contains code authored by new contributors:

  • All the authors are covered by a valid contributor assignment agreement,
    signed by the employer if needed, provided to ETH Zurich
  • The names of all the new contributors have been added to an updated
    version of the AUTHORS.rst file included in the PR

@petiaccja petiaccja added the module: backend Related to analysis/backend subpackages label Jun 24, 2022
@petiaccja petiaccja requested a review from havogt June 24, 2022 09:41
@petiaccja petiaccja force-pushed the compiled-backends-integration branch from 62b1f1f to da31c54 Compare July 4, 2022 14:55
@petiaccja petiaccja force-pushed the compiled-backends-integration branch 3 times, most recently from 4ac41c3 to 95b1302 Compare July 6, 2022 16:59
@petiaccja petiaccja force-pushed the compiled-backends-integration branch from 95b1302 to 1307bad Compare July 6, 2022 17:09
@petiaccja petiaccja changed the title WIP: Compiled backends integration Compiled backends integration Jul 6, 2022
.github/workflows/tox.yml Outdated Show resolved Hide resolved
.github/workflows/tox.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@havogt havogt left a comment

Choose a reason for hiding this comment

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

I played a bit with this PR and have some suggestions. I also found the place that breaks the shift (offset_provider) is not forwarded (and not checked that it is there)...

.github/workflows/tox.yml Outdated Show resolved Hide resolved
src/functional/fencil_processors/callables/cpp/build.py Outdated Show resolved Hide resolved
src/functional/fencil_processors/callables/cpp/build.py Outdated Show resolved Hide resolved
src/functional/fencil_processors/callables/cpp/build.py Outdated Show resolved Hide resolved
src/functional/fencil_processors/callables/cpp/build.py Outdated Show resolved Hide resolved
src/functional/fencil_processors/callables/cpp/build.py Outdated Show resolved Hide resolved
Copy link
Contributor

@egparedes egparedes left a comment

Choose a reason for hiding this comment

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

This is just a preliminar review. In general it looks good, but there are some points which I think need to be improved before merging. Some of them might look like cosmetic issues which can be added later but in my experience, if some basic foundation is not added at creation time, it's highly unlikely that they get added later (and it'd be much harder to do anyway):

  • Add standard headers to all the new files
  • Add a brief docstring to the modules and the main classes / functions
  • Add an ADR explaining the basic design of the toolchain and the pieces that need further work in the future.
  • Add some try / except blocks in the places most likely to fail with a meaningful error message. It doesn't need to be everywhere or superpolished message, but just something.

Regarding the folder structure, I would suggest to rename fencil_processors/callables to fencil_processors/common or fencil_processors/tools and move defs.py (why not definitions.py?) and cpp.py inside.

.github/workflows/tox.yml Outdated Show resolved Hide resolved
src/functional/fencil_processors/callables/cpp/bindings.py Outdated Show resolved Hide resolved
src/functional/fencil_processors/callables/cpp/bindings.py Outdated Show resolved Hide resolved
src/functional/fencil_processors/callables/cpp/bindings.py Outdated Show resolved Hide resolved
src/functional/fencil_processors/callables/cpp/bindings.py Outdated Show resolved Hide resolved
src/functional/fencil_processors/callables/modules.py Outdated Show resolved Hide resolved
src/functional/fencil_processors/callables/cpp/build.py Outdated Show resolved Hide resolved
src/functional/fencil_processors/callables/cache.py Outdated Show resolved Hide resolved
src/functional/fencil_processors/callables/modules.py Outdated Show resolved Hide resolved
Copy link
Contributor

@egparedes egparedes left a comment

Choose a reason for hiding this comment

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

It looks basically ready to go, the main issues have been solved. Just a couple of typos and some minor comments and optional suggestions.

petiaccja and others added 8 commits July 22, 2022 10:36
Co-authored-by: Enrique G. Paredes <18477+egparedes@users.noreply.github.com>
Co-authored-by: Enrique G. Paredes <18477+egparedes@users.noreply.github.com>
Co-authored-by: Enrique G. Paredes <18477+egparedes@users.noreply.github.com>
Copy link
Contributor

@egparedes egparedes left a comment

Choose a reason for hiding this comment

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

It looks good, thanks for the hard work! Just delete the unnecessary __init__.py in source_module_tests and it's good to go.

@petiaccja petiaccja merged commit 79b35d2 into functional Jul 22, 2022
@petiaccja petiaccja deleted the compiled-backends-integration branch July 22, 2022 13:53
@petiaccja petiaccja restored the compiled-backends-integration branch July 22, 2022 13:54
@havogt havogt deleted the compiled-backends-integration branch December 19, 2022 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: backend Related to analysis/backend subpackages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants