Py2fgen: fix fortran types, generate header only on demand#1275
Open
havogt wants to merge 9 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates py2fgen’s type/code generation to make boolean arguments C-interoperable end-to-end (C _Bool / Fortran logical(c_bool) / NumPy bool_) and refactors rendering/header emission so headers are generated/written only when explicitly requested (while keeping the generated .c self-contained for CFFI embedding).
Changes:
- Switch boolean codegen from
int/logical(c_int)to_Bool/logical(c_bool)and remove the legacy “bool-as-int32” conversion/copy paths. - Move the pure-string render API into
_generatorand make the generated.cembed the header content directly; add optional standalone header emission with include guards. - Update/extend tests and samples (new Fortran bool sample, CLI tests adjusted, snapshot/reference strategy updated for bindings).
Reviewed changes
Copilot reviewed 25 out of 27 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/tests/tools/py2fgen/wrappers/simple.py | Adds a bool scalar + bool array exported function to exercise the new bool ABI. |
| tools/tests/tools/py2fgen/test_render.py | Removes standalone render tests (migrated into test_generator.py). |
| tools/tests/tools/py2fgen/test_generator.py | Refactors imports and absorbs render tests; adjusts C generation expectations. |
| tools/tests/tools/py2fgen/test_conversion.py | Updates dtype mapping/tests for _Bool and removes int→bool conversion tests. |
| tools/tests/tools/py2fgen/test_codegen.py | Updates expected C/Fortran signatures for bools and adds include-guard tests. |
| tools/tests/tools/py2fgen/test_cli.py | Updates CLI expectations for “no header unless requested” and adds an end-to-end bool test. |
| tools/tests/tools/py2fgen/fortran_samples/test_bool.f90 | New Fortran sample validating bool scalar passing + bool array writeback. |
| tools/src/icon4py/tools/py2fgen/test_utils.py | Removes legacy bool→int32 conversion when building ArrayInfo. |
| tools/src/icon4py/tools/py2fgen/README.md | Documents new boolean requirements (c_bool / _Bool / bool_). |
| tools/src/icon4py/tools/py2fgen/_render.py | Deletes the old render module (functionality moved to _generator). |
| tools/src/icon4py/tools/py2fgen/_generator.py | Adds RenderedSources + render() and updates CFFI builder configuration. |
| tools/src/icon4py/tools/py2fgen/_conversion.py | Maps C _Bool to NumPy bool_ and removes bool copy/conversion logic. |
| tools/src/icon4py/tools/py2fgen/_codegen.py | Updates bool type mappings, adjusts Fortran declarations, and adds include guards. |
| tools/src/icon4py/tools/py2fgen/_cli.py | Changes --skip-compilation behavior/logging to generate .c only (no .h). |
| tools/src/icon4py/tools/py2fgen/init.py | Re-exports render/RenderedSources from _generator (drops _render). |
| bindings/tests/bindings/test_codegen_references.py | Switches snapshot generation to all_bindings outputs and avoids snapshotting .c. |
| bindings/tests/bindings/references/icon4py_bindings.h | Updates reference header with include guard and _Bool signatures. |
| bindings/tests/bindings/references/grid/grid.py | Removes per-binding reference artifact (superseded by combined binding references). |
| bindings/tests/bindings/references/grid/grid.h | Same as above (removed). |
| bindings/tests/bindings/references/grid/grid.f90 | Same as above (removed). |
| bindings/tests/bindings/references/diffusion/diffusion.py | Removes per-binding reference artifact (superseded). |
| bindings/tests/bindings/references/diffusion/diffusion.h | Same as above (removed). |
| bindings/tests/bindings/references/diffusion/diffusion.f90 | Same as above (removed). |
| bindings/tests/bindings/.gitignore | Simplifies ignore rules to cover the new references_new/** outputs. |
| bindings/src/icon4py/bindings/all_bindings.py | Adds explicit --output-* options and makes .h emission optional/on-demand. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The element type is inferred from the CFFI pointer, so `dtype` is removed from `as_array` and all callers (`_as_array_mapping`, `default_mapping`, the generated wrapper, `icon4py_export._as_field`); also drops the now-unused `_definitions` import from the generated wrapper. Document why the include guard needs no `library_name` sanitization. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
cscs-ci run default |
Contributor
Author
|
cscs-ci run distributed |
These still passed the dtype argument that was dropped from as_array; only the cscs/default datatests exercise them, so it slipped past the GitHub CI checks. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- drop dead BUILTIN_TO_NUMPY_TYPE / to_np_type - write all_bindings outputs via a loop - _unpack_numpy raises on unknown C type (consistent with _unpack_cupy) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…dling_of_logicals # Conflicts: # tools/tests/tools/py2fgen/test_generator.py
…dling_of_logicals
|
Mandatory Tests Please make sure you run these tests via comment before you merge!
Optional Tests To run benchmarks you can use:
To run tests and benchmarks with the DaCe backend you can use:
To run test levels ignored by the default test suite (mostly simple datatest for static fields computations) you can use:
For more detailed information please look at CI in the EXCLAIM universe. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.