You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Adds a central, overridable option to control whether gt4py.next runs source formatters (black, clang-format) on generated code: config.FORMAT_SOURCES, initialized from GT4PY_FORMAT_SOURCES and defaulting to the master GT4PY_DEBUG flag. Formatters are therefore only used by default in debug mode.
The value is captured when a code spec or workflow step is created, not read at formatting time, so no non-local context influences the generated source outside of the fingerprints/cache keys:
SourceCodeSpec.format_source (new field, default_factory reading config.FORMAT_SOURCES); artifacts.format_source() is a no-op when it is false. Nanobind bindings follow the program's spec.
GTFNTranslationStep resolves its default code spec in __post_init__, so the setting is part of the step fingerprint and hence of the persistent translation cache key. An explicit spec not matching the device type now raises a ValueError.
Roundtrip.format_source (new field) replaces debug as the formatting switch and is part of the in-process source cache key; debug only controls writing the temporary .py file.
SDFGCodeSpec pins format_source=False (there is no SDFG formatter), and the compiledb prototype normalizes it, so neither DaCe build folders nor the compiledb cache split on the setting.
Removed the redundant clang-format pass in generate_stencil_source (the full file was formatted again afterwards); the format_cpp program formatter always formats explicitly, since producing human-readable code is its purpose.
Behaviour changes
Generated C++/CUDA/HIP sources, nanobind bindings and roundtrip Python are no longer formatted by default (previously gtfn formatted whenever clang-format was installed). Use GT4PY_DEBUG=1 or GT4PY_FORMAT_SOURCES=1 to get formatted sources.
Roundtrip(debug=True) alone no longer formats.
Backends built at import time (run_gtfn, roundtrip.default, ...) capture the value at import: use the env var, or build a new backend / pass an explicit spec (e.g. code_spec=CPPCodeSpec(format_source=True)) to override it.
GTFNTranslationStep now holds a resolved spec: step.replace(device_type=...) must also pass code_spec=None, and steps are no longer hashable (the spec carries formatter_options as a dict; Backend/OTFCompileWorkflow were already unhashable).
gt4py.cartesian is unaffected (it keeps its per-stencil format_source build option).
Testing
New unit tests for spec defaults/fingerprints, SDFGCodeSpec, artifacts.format_source, gtfn step spec resolution and device mismatch, compiledb prototype key stability, and roundtrip formatting (direct and through Roundtrip.__call__).
tests/next_tests/unit_tests pass; gtfn (CPU) + roundtrip on ffront_tests/test_scan.py and test_program.py pass both with the default and with GT4PY_FORMAT_SOURCES=1 and clang-format available.
Requirements
All fixes and/or new features come with corresponding tests.
Important design decisions have been documented in the appropriate ADR inside the docs/development/ADRs/ folder. (Not needed: new config option, no architectural change.)
… generated code
Add `config.FORMAT_SOURCES` (env var `GT4PY_FORMAT_SOURCES`, defaulting to
`GT4PY_DEBUG`) to control whether generated sources are run through
black/clang-format. The value is captured at construction time in
`SourceCodeSpec.format_source` and `Roundtrip.format_source`, so it is part
of the fingerprints and cache keys of specs and workflow steps.
- `GTFNTranslationStep` resolves its default code spec in `__post_init__`
so the setting is part of the translation cache key.
- Drop the redundant clang-format pass in `generate_stencil_source`;
`format_cpp` always formats explicitly.
- `SDFGCodeSpec` pins `format_source=False` and the compiledb prototype
normalizes it, so neither DaCe builds nor the compiledb cache split on it.
- Restructure artifacts.format_source for readability.
- Normalize format_source for the compiledb prototype at the call site.
- Compute all device-dependent settings of GTFNTranslationStep in __post_init__.
format_source is still an initializer argument here, so SDFGCodeSpec(format_source=True) is accepted even though SDFG has no formatter. Passing that spec to artifacts.format_source() then reaches its assertion at artifacts.py:130 instead of remaining pinned to False; make this field non-init or explicitly reject True.
Missing test for gtfn.format_cpp formatting composition
This new unconditional formatting path is not exercised by the added tests: they cover _generate_source() and artifacts.format_source(), but no test invokes program_processors.formatters.gtfn.format_cpp(). A regression in the direct generate_stencil_source() plus formatter composition (including the intended single formatting pass) would therefore go unnoticed; add a focused test for this formatter.
Addressed the two items from the latest Copilot overview in 845f05f:
SDFGCodeSpec.format_source is now field(default=False, init=False), so SDFGCodeSpec(format_source=True) raises a TypeError and can never reach the missing-formatter assertion. New test: test_sdfg_code_spec_rejects_format_source.
New test_format_cpp_always_formats_once calls formatters.gtfn.format_cpp with FORMAT_SOURCES both on and off, and checks that it formats exactly once with ("cpp", style="LLVM"), so a reintroduced inner formatting pass in generate_stencil_source would fail it.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
Unresolved test coverage gaps remain for configuration precedence, binding formatting, replacement semantics, device mappings, and cache behavior.
Review effort: Lite Findings: None
Previously missed (1)
In code that hasn't changed since last review
Missing tests for GT4PY_FORMAT_SOURCES precedence and fallback
src/gt4py/next/config.py:113
The new GT4PY_FORMAT_SOURCES initialization path is not covered by tests: tests/next_tests/unit_tests/test_config.py:28-48 exercises env_flag_to_bool with a dummy variable, but never verifies this module-level value honors an explicit GT4PY_FORMAT_SOURCES value or falls back to DEBUG when the variable is unset. Add isolated import/reload (or subprocess) coverage for both precedence cases so this user-facing configuration cannot regress.
Added test_format_sources_precedence in 07d1973 to cover the one concrete item in the latest Copilot overview: an explicit GT4PY_FORMAT_SOURCES wins over GT4PY_DEBUG in both directions, and the flag falls back to GT4PY_DEBUG when it is unset. The test runs a fresh copy of config.py for each env combination, so the real gt4py.next.config module is left untouched. The overview's other bullets list no concrete findings; replacement semantics, device mappings and cache behaviour already have tests in test_gtfn_module.py, test_compiledb.py and test_roundtrip.py.
… eve.formatting (#2927)
## Description
Removes automatic formatting (black / clang-format) of generated code
from `gt4py.next` and keeps it only for `gt4py.cartesian` (and tests),
with the formatter reduced to a minimal best-effort module.
- New leaf module `gt4py.eve.formatting` with `format_python_source`
(black; `line_length` argument, default 100; black's target version
pinned to the running interpreter) and `format_cpp_source`
(clang-format, LLVM style); both return the input unchanged when the
tool is missing or fails. The executable can still be overridden with
`CLANG_FORMAT_EXECUTABLE`.
- Removed the old formatting API from `gt4py.eve.codegen`
(`format_source`, `format_python_source`, `format_cpp_source`,
`register_formatter`, `SOURCE_FORMATTERS`, `FormattingError`,
`FormatterNameError`). Importing `eve.codegen` no longer imports black
nor spawns a clang-format subprocess.
- `gt4py.next`: no formatting at all. `SourceCodeSpec` and subclasses
lose `formatter_key`/`formatter_options` (specs are now hashable);
`otf.artifacts.format_source` removed; gtfn sources, nanobind bindings
and roundtrip debug output are emitted unformatted;
`formatters.gtfn.format_cpp` returns raw C++. Roundtrip: `debug` removed
from the source-cache key and added to the module-cache key (debug runs
still write the temp `.py` file).
- `gt4py.cartesian`: no behaviour change. It only switches its existing
formatting calls from `eve.codegen.format_source(...)` to
`eve.formatting.format_cpp_source(...)` / `format_python_source(...)`;
the call sites, the `format_source` option handling and the line lengths
stay as they were.
- `tach.toml`: `gt4py.eve.formatting` is its own module and only
`gt4py.cartesian` may depend on it.
- Dependencies: `black` is no longer a runtime dependency and
`clang-format` moved out of the `standard` extra; both are now in the
`cartesian` extra and the `test` dependency group. This is the only
change visible from `gt4py.cartesian`: a plain `pip install gt4py` no
longer formats cartesian output.
- Supersedes #2919.
### Breaking changes / notes
- Removed `gt4py.eve.codegen` formatting API; use
`gt4py.eve.formatting`. Known downstream user: icon4py
`tools/src/icon4py/tools/py2fgen/_codegen.py` (also needs `black` as its
own dependency).
- `black`/`clang-format` are no longer installed by default (`pip
install gt4py[cartesian]` to get formatted cartesian output).
- Generated gtfn C++ include order is no longer sorted by clang-format;
CPU is covered by CI, GPU is covered only by CSCS CI.
## Requirements
- [x] All fixes and/or new features come with corresponding tests. (New
`tests/eve_tests/unit_tests/test_formatting.py`; existing next binding
tests updated; new roundtrip module-cache test.)
- [x] Important design decisions have been documented in the appropriate
ADR inside the [docs/development/ADRs/](docs/development/ADRs/README.md)
folder. (No new ADR needed; one-line note added to ADR 0012.)
If this PR contains code authored by new contributors please make sure:
- [ ] The PR contains an updated version of the `AUTHORS.md` file adding
the names of all the new contributors. (N/A)
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
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.
egparedes
left a comment
havogt
left a comment
Description
Adds a central, overridable option to control whether
gt4py.nextruns source formatters (black, clang-format) on generated code:config.FORMAT_SOURCES, initialized fromGT4PY_FORMAT_SOURCESand defaulting to the masterGT4PY_DEBUGflag. Formatters are therefore only used by default in debug mode.The value is captured when a code spec or workflow step is created, not read at formatting time, so no non-local context influences the generated source outside of the fingerprints/cache keys:
SourceCodeSpec.format_source(new field,default_factoryreadingconfig.FORMAT_SOURCES);artifacts.format_source()is a no-op when it is false. Nanobind bindings follow the program's spec.GTFNTranslationStepresolves its default code spec in__post_init__, so the setting is part of the step fingerprint and hence of the persistent translation cache key. An explicit spec not matching the device type now raises aValueError.Roundtrip.format_source(new field) replacesdebugas the formatting switch and is part of the in-process source cache key;debugonly controls writing the temporary.pyfile.SDFGCodeSpecpinsformat_source=False(there is no SDFG formatter), and the compiledb prototype normalizes it, so neither DaCe build folders nor the compiledb cache split on the setting.generate_stencil_source(the full file was formatted again afterwards); theformat_cppprogram formatter always formats explicitly, since producing human-readable code is its purpose.Behaviour changes
GT4PY_DEBUG=1orGT4PY_FORMAT_SOURCES=1to get formatted sources.Roundtrip(debug=True)alone no longer formats.run_gtfn,roundtrip.default, ...) capture the value at import: use the env var, or build a new backend / pass an explicit spec (e.g.code_spec=CPPCodeSpec(format_source=True)) to override it.GTFNTranslationStepnow holds a resolved spec:step.replace(device_type=...)must also passcode_spec=None, and steps are no longer hashable (the spec carriesformatter_optionsas a dict;Backend/OTFCompileWorkflowwere already unhashable).gt4py.cartesianis unaffected (it keeps its per-stencilformat_sourcebuild option).Testing
SDFGCodeSpec,artifacts.format_source, gtfn step spec resolution and device mismatch, compiledb prototype key stability, and roundtrip formatting (direct and throughRoundtrip.__call__).tests/next_tests/unit_testspass; gtfn (CPU) + roundtrip onffront_tests/test_scan.pyandtest_program.pypass both with the default and withGT4PY_FORMAT_SOURCES=1and clang-format available.Requirements