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

refactor[next]: Refactor workflow based executors and move the backend class. #1470

Merged
merged 2 commits into from
Feb 29, 2024

Conversation

DropD
Copy link
Contributor

@DropD DropD commented Feb 26, 2024

Description

Changed

  • OTFCompileExecutor and CachedOTFCompileExecutor merged into ModularExecutor

Moved

  • OTFBackend moved and renamed to next.backend.Backend

Reasoning

The two *CompileExecutor classes were identical, except for type hints. Since they are now almost exclusively used inside OTFBackend, which does not retain typing information about which of them it contains, this distinction is no longer helpful for static type checking.

OTFBackend has always been more general than it's naming and location suggested. It is the de-facto definition of a backend within gt4py.next: a wrapper around an executor and an allocator. This change makes the status quo visible.

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.

Copy link
Contributor

@tehrengruber tehrengruber left a comment

Choose a reason for hiding this comment

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

I think it makes sense. From a quick look it seems that Backend.executor could directly become otf_workflow as the only task ModularExecutor has is to transform the args of its __call__ method into a form that is understood by otf_workflow. This could equally be done by Backend.__call__. Since this involves some minor refactoring in other parts of the codebase that rely on Backend.executor we can do it in another PR (maybe if there are some free cycles sometime).

@DropD
Copy link
Contributor Author

DropD commented Feb 29, 2024

I think it makes sense. From a quick look it seems that Backend.executor could directly become otf_workflow as the only task ModularExecutor has is to transform the args of its __call__ method into a form that is understood by otf_workflow. This could equally be done by Backend.__call__. Since this involves some minor refactoring in other parts of the codebase that rely on Backend.executor we can do it in another PR (maybe if there are some free cycles sometime).

I think it makes sense. From a quick look it seems that Backend.executor could directly become otf_workflow as the only task ModularExecutor has is to transform the args of its __call__ method into a form that is understood by otf_workflow. This could equally be done by Backend.__call__. Since this involves some minor refactoring in other parts of the codebase that rely on Backend.executor we can do it in another PR (maybe if there are some free cycles sometime).

My first attempt was to do just that. It turned out to be a much bigger refactoring because the assumption that Backend.executor exists is in many test utils. So I opted for the current PR as a first step.

The current cycle will take us a bit closer still.

@DropD DropD merged commit d7b6562 into GridTools:main Feb 29, 2024
39 checks passed
@DropD DropD deleted the exp-refac-modexec branch February 29, 2024 16:01
@tehrengruber
Copy link
Contributor

👍 Maybe a todo would be useful in the future, if not for yourself for another person stumbling upon it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants