refactor[next]: workflowify step3 #1516

DropD · 2024-04-02T11:49:57Z

Description

New:

ffront.stages.FieldOperatorDefinition
- all the data to start the toolchain from a field operator dsl definition
ffront.stages.FoastOperatorDefinition
- data after lowering from field operator dsl code
ffront.stages.FoastWithTypes
- program argument types in addition to the foast definition for creating a program AST
ffront.stages.FoastClosure
- program arguments in addition to the foast definition, ready to run the whole toolchain

Changed:

decorator.Program.__post_init__
- implementation moved to past_passes.linters workflow steps
- linting stage added to program transforms
decorator.FieldOperator.from_function
- implementation moved to workflow step in ffront.func_to_foast
decorator.FieldOperator.as_program
- implementation moved to workflow steps in ffront.foast_to_past
decorator.FieldOperator data attributes
- added: definition_stage
- removed:
  - .foast_node: replaced with .foast_stage.foast_node
  - .definition: replaced with .definition_stage.definition
next.backend.Backend
- renamed: .transformer -> .transforms_prog
- added: .transforms_fop, toolchain for starting from field operator
otf.recipes.FieldOpTransformWorkflow
- now has all the steps from DSL field operator to ProgramCall via foast_to_past, with additional steps to go to the field operator IteratorIR expression directly instead (not run by default). The latter foast_to_itir step is required during lowering of programs that call a field operator.

Requirements

All fixes and/or new features come with corresponding tests.
Important design decisions have been documented in the approriate ADR inside the docs/development/ADRs/ folder.

havogt

I feel you need to walk me through these changes, specifically the 2 different DEFAULT_X_TRANSFORMS and backend specifying 2 kinds of transforms.

src/gt4py/next/otf/recipes.py

havogt

Let's talk with Enrique about the hashing part.

havogt · 2024-04-16T07:39:49Z

src/gt4py/next/ffront/decorator.py

+        if self.backend is not None and self.backend.transforms_prog is not None:
+            return self.backend.transforms_prog.func_to_past(self.definition_stage)
+        return next_backend.DEFAULT_PROG_TRANSFORMS.func_to_past(self.definition_stage)


I should have asked this question, in the previous refactoring: what's happening here? Can you document it? What's the case when the DEFAULT_PROG_TRANSFORMS should be used?

src/gt4py/next/ffront/decorator.py

havogt · 2024-04-16T07:50:16Z

src/gt4py/next/ffront/stages.py

+    """
+    Allows deriving dataclasses to modify what goes into the content hash per-field.
+
+    Warning: Using this will modify how the class gets pickled. If unpickling is desired,


@egparedes Can you have a look at this part? I am not sure I understand its implication and it seems like a second pair of eyes make sense.

IIUC, the Mixin classes and the functions have been added here to support hashing the Stage classes with eve.content_hash but I don't really see why this is needed. I mean, if these classes need to be hashable and there is an unique way to implement the hash that makes sense in all use cases, then we could just write a custom __hash__ instead of hacking around with __setstate__ and __getstate__ methods (which in addition of breaking the pickleablity of the instances, it'd only work for the content_hash function).

An even better and more general solution, in my opinion, would be to define custom hash functions as required for different purposes as singledispatch free functions, and then register specific implementations for the classes that need to be hashable in that specific way.

I think the singledispatch function is probably the solution.

My solution is up for review.

src/gt4py/next/backend.py

havogt

Looks good to me. Let's wait for @egparedes feedback on the hashing.

src/gt4py/next/backend.py

egparedes

I only reviewed the changes in stages.py related to hashing. The code itself looks good, but I strongly suggest to rename the functions.

src/gt4py/next/ffront/stages.py

egparedes · 2024-04-22T14:23:33Z

src/gt4py/next/ffront/stages.py

+@functools.singledispatch
+def add_content_to_fingerprint(obj: Any, hasher: xtyping.HashlibAlgorithm) -> None:
+    hasher.update(str(obj).encode())
+
+
+@add_content_to_fingerprint.register(FieldOperatorDefinition)
+@add_content_to_fingerprint.register(FoastOperatorDefinition)
+@add_content_to_fingerprint.register(FoastWithTypes)
+@add_content_to_fingerprint.register(FoastClosure)
+@add_content_to_fingerprint.register(ProgramDefinition)
+@add_content_to_fingerprint.register(PastProgramDefinition)
+@add_content_to_fingerprint.register(PastClosure)
+def add_content_to_fingerprint_stages(obj: Any, hasher: xtyping.HashlibAlgorithm) -> None:
+    add_content_to_fingerprint(obj.__class__, hasher)
+    for field in dataclasses.fields(obj):
+        add_content_to_fingerprint(getattr(obj, field.name), hasher)
+
+
+@add_content_to_fingerprint.register
+def add_str_to_fingerprint(obj: str, hasher: xtyping.HashlibAlgorithm) -> None:
+    hasher.update(str(obj).encode())
+
+
+@add_content_to_fingerprint.register(int)
+@add_content_to_fingerprint.register(bool)
+@add_content_to_fingerprint.register(float)
+def add_builtin_to_fingerprint(
+    obj: None,
+    hasher: xtyping.HashlibAlgorithm,
+) -> None:
+    hasher.update(str(obj).encode())


A suggestion to reduce boilerplate:

Suggested change

@functools.singledispatch

def add_content_to_fingerprint(obj: Any, hasher: xtyping.HashlibAlgorithm) -> None:

hasher.update(str(obj).encode())

@add_content_to_fingerprint.register(FieldOperatorDefinition)

@add_content_to_fingerprint.register(FoastOperatorDefinition)

@add_content_to_fingerprint.register(FoastWithTypes)

@add_content_to_fingerprint.register(FoastClosure)

@add_content_to_fingerprint.register(ProgramDefinition)

@add_content_to_fingerprint.register(PastProgramDefinition)

@add_content_to_fingerprint.register(PastClosure)

def add_content_to_fingerprint_stages(obj: Any, hasher: xtyping.HashlibAlgorithm) -> None:

add_content_to_fingerprint(obj.__class__, hasher)

for field in dataclasses.fields(obj):

add_content_to_fingerprint(getattr(obj, field.name), hasher)

@add_content_to_fingerprint.register

def add_str_to_fingerprint(obj: str, hasher: xtyping.HashlibAlgorithm) -> None:

hasher.update(str(obj).encode())

@add_content_to_fingerprint.register(int)

@add_content_to_fingerprint.register(bool)

@add_content_to_fingerprint.register(float)

def add_builtin_to_fingerprint(

obj: None,

hasher: xtyping.HashlibAlgorithm,

) -> None:

hasher.update(str(obj).encode())

@functools.singledispatch

def add_content_to_fingerprint(obj: Any, hasher: xtyping.HashlibAlgorithm) -> None:

hasher.update(str(obj).encode())

_default_impl = add_content_to_fingerprint[object]

for t in (str, int, bool, float):

add_content_to_fingerprint.register(t, _default_impl)

@add_content_to_fingerprint.register(FieldOperatorDefinition)

@add_content_to_fingerprint.register(FoastOperatorDefinition)

@add_content_to_fingerprint.register(FoastWithTypes)

@add_content_to_fingerprint.register(FoastClosure)

@add_content_to_fingerprint.register(ProgramDefinition)

@add_content_to_fingerprint.register(PastProgramDefinition)

@add_content_to_fingerprint.register(PastClosure)

def add_content_to_fingerprint_stages(obj: Any, hasher: xtyping.HashlibAlgorithm) -> None:

add_content_to_fingerprint(obj.__class__, hasher)

for field in dataclasses.fields(obj):

add_content_to_fingerprint(getattr(obj, field.name), hasher)

I think registering a dispatch for the builtins is not even necessary at this point, now that the default is exactly the same.

It was necessary for str and int, but only to avoid a max recursion depth error. I am not exactly sure why this helps.

For str is needed because otherwise it will call the implementation for Iterable, not the default implementation, since str is Iterable (and that's why the infinite recursion appears). For int, I don't see why it would be needed. Are you sure that just str is not enough? Maybe I'm misunderstanding something...

egparedes

I still wonder why it is needed to register a specific implementation for int in the add_content_to_fingerprint singledispatch function, but other than that, the fingerprinting functions look good to me.

DropD · 2024-04-29T13:44:55Z

I still wonder why it is needed to register a specific implementation for int in the add_content_to_fingerprint singledispatch function, but other than that, the fingerprinting functions look good to me.

I also wonder why that should impact the recursion depth.

DropD added 13 commits March 26, 2024 16:57

workflowify past linting and args injection

b0f7e3a

workflowify func -> FOAST

445b35d

fix missing attribute rename.

3561a0c

workflowify FieldOperator.as_program

2842cad

make sure scan operator attributes are properly hashed

3357d11

Merge branch 'main' into c20-workflowify-step3

3aab3c8

Merge branch 'main' into c20-workflowify-step3

44b3060

add closure vars to hash for program definition

ebf164c

[wip] integrating fieldop workflows

d3a7851

update foast pretty printer doctest

a99fc48

fix code quality issues

c71e0bf

reuse content hashing code in ffront.stages

df648e8

remove erroneously committed .python-version

1f5c336

DropD marked this pull request as ready for review April 5, 2024 09:20

DropD requested a review from havogt April 8, 2024 13:03

DropD added 2 commits April 8, 2024 15:29

Merge remote-tracking branch 'upstream/main' into c20-workflowify-step3

2cab5b7

re-apply Program.itir fix after merge

0a54c3d

havogt reviewed Apr 9, 2024

View reviewed changes

src/gt4py/next/otf/recipes.py Outdated Show resolved Hide resolved

havogt reviewed Apr 10, 2024

View reviewed changes

src/gt4py/next/otf/recipes.py Outdated Show resolved Hide resolved

DropD added 3 commits April 11, 2024 11:53

move backend transforms to next.backend

e1ed8d2

add tested toolchain workthrough notebook

3e30ca2

fix toolchain walkthrough notebook

9e50bd8

havogt requested changes Apr 16, 2024

View reviewed changes

havogt reviewed Apr 16, 2024

View reviewed changes

src/gt4py/next/backend.py Outdated Show resolved Hide resolved

DropD added 6 commits April 18, 2024 10:16

put default toolchain steps into definitions

678bd67

replace content_hash with dedicated cache key gen for ffront stages

fad81c0

add typeignores for hash algorithms

af1a016

downgrade singledispatch type hints for py < 310

556e8c5

docstrings for AST based decorator wrappers

fba07f1

todos for linting step calls in decorator wrappers

50bdea2

comment first occurrence of backwards compat backend pattern

63121fd

DropD requested review from egparedes and havogt April 19, 2024 08:02

havogt approved these changes Apr 19, 2024

View reviewed changes

src/gt4py/next/backend.py Show resolved Hide resolved

src/gt4py/next/backend.py Show resolved Hide resolved

stages hasher: avoid recursing into non-stage dataclasses

04e407b

DropD mentioned this pull request Apr 19, 2024

refactor[next]: Flexible embedded backends #1535

Closed

4 tasks

Merge remote-tracking branch 'upstream/main' into c20-workflowify-step3

ba838f1

egparedes requested changes Apr 22, 2024

View reviewed changes

src/gt4py/next/ffront/stages.py Outdated Show resolved Hide resolved

src/gt4py/next/ffront/stages.py Outdated Show resolved Hide resolved

src/gt4py/next/ffront/stages.py Outdated Show resolved Hide resolved

src/gt4py/next/ffront/stages.py Outdated Show resolved Hide resolved

DropD added 3 commits April 22, 2024 10:35

rename ffront.stages.cache_key -> ffront.stages.fingerprint_stage

9bb341a

improve ffront.stage fingerprinting

888017d

update HashlibAlgorithm in eve

345ce8e

egparedes reviewed Apr 22, 2024

View reviewed changes

DropD added 2 commits April 23, 2024 13:28

remove redundant singledispatch methods

7be23de

escape the recursion depth hammer when hashing stages

73c3d18

egparedes approved these changes Apr 23, 2024

View reviewed changes

DropD merged commit 9cc7548 into GridTools:main Apr 29, 2024
51 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor[next]: workflowify step3 #1516

refactor[next]: workflowify step3 #1516

DropD commented Apr 2, 2024 •

edited

Loading

havogt left a comment

havogt left a comment

havogt Apr 16, 2024

havogt Apr 16, 2024

egparedes Apr 17, 2024

DropD Apr 17, 2024

DropD Apr 19, 2024

havogt left a comment

egparedes left a comment

egparedes Apr 22, 2024

DropD Apr 22, 2024

DropD Apr 23, 2024

egparedes Apr 23, 2024

egparedes left a comment

DropD commented Apr 29, 2024

refactor[next]: workflowify step3 #1516

refactor[next]: workflowify step3 #1516

Conversation

DropD commented Apr 2, 2024 • edited Loading

Description

New:

Changed:

Requirements

havogt left a comment

Choose a reason for hiding this comment

havogt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

havogt left a comment

Choose a reason for hiding this comment

egparedes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egparedes left a comment

Choose a reason for hiding this comment

DropD commented Apr 29, 2024

DropD commented Apr 2, 2024 •

edited

Loading