-
Notifications
You must be signed in to change notification settings - Fork 54
refactor[next]: Rename and refactor toolchain type definitions #2474
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
Conversation
Bug fixes for tests
7664c1c to
966a72f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This is a comprehensive refactoring pull request that renames and reorganizes the entry point types and workflow stages in the GT4Py Next toolchain. The main goals are to improve naming clarity and better organize the codebase structure.
Changes:
- Renamed
CompilableProgramtoConcreteArtifactto better reflect its generic nature - Renamed
CompilableSourcetoCompilableProjectto emphasize it's a complete project - Renamed stage definition classes with "Def" suffix instead of "Definition" for consistency
- Moved step type protocols from
otf.step_typestootf.definitionsmodule - Moved
jit_to_aot_argsfunction fromotf.argumentstobackendmodule - Updated all type aliases and references throughout the codebase
Reviewed changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/gt4py/next/otf/toolchain.py |
Core refactoring: CompilableProgram → ConcreteArtifact, updated type variables |
src/gt4py/next/otf/stages.py |
Removed old CompilableProgram alias, renamed CompilableSource → CompilableProject |
src/gt4py/next/otf/definitions.py |
New home for step protocols, added CompilableProgramDef and related type aliases |
src/gt4py/next/ffront/stages.py |
Renamed all stage classes with "Def" suffix, changed collections → collections.abc |
src/gt4py/next/ffront/decorator.py |
Updated mixin class name, removed unused Generic parameters, added GTEntryPoint type alias |
src/gt4py/next/ffront/field_operator_ast.py |
Added OperatorNode type alias |
src/gt4py/next/backend.py |
Moved jit_to_aot_args here, updated all type references |
src/gt4py/next/typing.py |
Added GTEntryPoint type alias |
| Various processor files | Updated type references to use new names |
| Test files | Updated to use new type names |
docs/user/next/advanced/HackTheToolchain.md |
Updated examples with new type names (incomplete) |
Comments suppressed due to low confidence (3)
src/gt4py/next/typing.py:37
- The new type alias
GTEntryPointis defined at line 20 but is not exported in the__all__list. If this type is intended for external use, it should be added to__all__to make it part of the public API.
src/gt4py/next/otf/definitions.py:66 - The docstring at line 66 still references "CompilableSource" which was renamed to "CompilableProject". The docstring should be updated to: "Compile program source code and bindings into a python callable (CompilableProject -> CompiledProgram)."
tests/next_tests/unit_tests/program_processor_tests/codegens_tests/gtfn_tests/test_gtfn_module.py:82 - Call to a non-callable of class GTFNTranslationStepFactory.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.../next_tests/unit_tests/program_processor_tests/codegens_tests/gtfn_tests/test_gtfn_module.py
Outdated
Show resolved
Hide resolved
.../next_tests/unit_tests/program_processor_tests/codegens_tests/gtfn_tests/test_gtfn_module.py
Outdated
Show resolved
Hide resolved
.../next_tests/unit_tests/program_processor_tests/codegens_tests/gtfn_tests/test_gtfn_module.py
Outdated
Show resolved
Hide resolved
| class CompilableProgram(Generic[PrgT, ArgT]): | ||
| data: PrgT | ||
| args: ArgT | ||
| class ConcreteArtifact(Generic[DefT, ArgsT]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name seems the most appropriate for me since this is a super-generic class (this module seems to define only infrastructure to build super-generic pipelines of workflows). The concrete useful types for our compilation toolchain are defined in definitions. A problem I see is that in the actual code this class is used a lot to create instances, which might be confusing. Ideally we should use the more meaningful type aliases instead but I would leave it as it is in this PR and propose to incrementally replace the references with the appropriate concrete definitions in future PRs.
| # TODO(ricoh): reconsider name in view of future backends producing standalone compilable ProgramSource code | ||
| @dataclasses.dataclass(frozen=True) | ||
| class CompilableSource(Generic[SrcL, SettingT, TgtL]): | ||
| class CompilableProject(Generic[SrcL, SettingT, TgtL]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this class contains actual code ready to be compiled, so it doesn't use the Def sufffix, which is used for classes containing different forms of IR or DSL definitions from which actual source code artifacts like this will be generated.
|
|
||
|
|
||
| @dataclasses.dataclass(frozen=True) | ||
| class StripArgsAdapter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stripping part is not really expressed here, right? It's just a generic transformation from ConcreteArtifact to anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stripping is implemented below in line 65, where ONLY the inp.data is passed on. Or in your terms, it's a generic transformation from ConcreteArtifact to anything with the limitation that only the .data attribute of the ConcreteArtifact can be used in the transformation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"strip the ConcreteArtifact object of the .args, pass on the rest". Maybe a better name would be "data pass filter"? since only the data is passed on?
havogt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice if you can add description of how prefix/sufix are used, e.g. Def etc.
| class _CompilableGTEntryPointMixin(Generic[ffront_stages.DSLDefinitionT]): | ||
| """ | ||
| Mixing used by program and program-like objects. | ||
|
|
||
| Contains functionality and configuration options common to all kinds of program-likes. | ||
| """ | ||
|
|
||
| definition_stage: ProgramLikeDefinitionT | ||
| definition_stage: ffront_stages.DSLDefinitionT | ||
| backend: Optional[next_backend.Backend] | ||
| compilation_options: options.CompilationOptions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't recognize this class in the first place, so I have to withold judgement on this one. The docstring is a bit on the cryptic side and it's for some reason prefixed with a _.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's purpose seems to be that classes that derive from it can be directly used as compilation stages, without being wrapped. If that's what it does, the new name fits better. Though i am not convinced by the leading underscore, but our coding guidelines are silent about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class was recently introduced (see #2368) as a mixin class to support fast-path execution (using a CompiledProgramsPool) of both compiled programs and field operators with variants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is more internal coherence in the naming now, and overall it's an improvement.
The only thing where the naming isn't a strict improvement is where otf.step_types is renamed to otf.definitions and some type aliases added, while the otf.stages, which are definitions as well remain separate.
So here we went from two concretely named separate modules to a genericly named module and an inexplicably separate concretely named one.
I agree that |
Renaming and slight refactoring of type definitions used in the
gt4py.nexttoolchain. The main goals are to improve naming clarity and organize better the codebase.Main changes:
otf.step_typestootf.definitionsand extended with more concrete type definitionstoolchain.CompilableProgramtotoolchain.ConcreteArtifactto better reflect its generic natureCompilableSourcetoCompilableProjectto emphasize it's a complete projectotf.argumentstootf.backend