fix: DyCore orchestration without timer allocation #119
Conversation
romanc
changed the title
We can't do any allocation in DaCe code (for good reasons). This PR fixes fallout from NOAA-GFDL#97 where we accidentally introduced the allocaiton of `NullTimer()` in DaCe-owned code. This PR moves the construction of the `Timer` up to calling code. Unrelated, the PR reduces the indet level by adding early returns and adds a couple of type hints here and there.
romanc
force-pushed
the
romanc/orch-fv-dynamics-no-alloc-in-dace
branch
from
4d300ce to
8929dd7
Compare
romanc
left a comment
romanc
left a comment
There was a problem hiding this comment.
The PR contain a couple small unrelated changes, which are explained in this self-review.
| self._compute(state, timer) | ||
| self._checkpoint_fvdynamics(state=state, tag="Out") | ||
|
|
||
| def compute_preamble(self, state: DycoreState, is_root_rank: bool): |
There was a problem hiding this comment.
unrelated: is_root_rank is unused and can thus be removed
| "Dynamical Core (fv_dynamics): Adiabatic with positive kord_tm" | ||
| " is not implemented." | ||
| ) | ||
| else: |
There was a problem hiding this comment.
unrelated: no else needed because the if branch just raises
| ) | ||
|
|
||
| def __call__(self, *args, **kwargs): | ||
| return self.step_dynamics(*args, **kwargs) |
There was a problem hiding this comment.
unrelated: no need to explicitly return from the __call__(..) function. step_dynamics(..) also doesn't return any value.
FlorianDeconinck
left a comment
FlorianDeconinck
left a comment
There was a problem hiding this comment.
So you removed the optional Timer and is forcing user to pass in a Timer. It doesn't match the original idea.
@twicki : opinion on this? I could go both way, though forcing user to learn about NullTimer for a simple run might be a bit of a stretch
Side note: the timing should be handled via the NDSLRuntime in a non destructive way, know that we have some form of control over runtime code.
Tell me to change it and I will. This seemed a lot simpler, especially given the context that we are likely to re-do timers based on |
Good point, keep it as you change it as indeed this needs work to be transparent to users rather than in code. |
2 participants
Description
We can't do any allocation in DaCe code (for good reasons). This PR fixes fallout from #97 where we accidentally introduced the allocaiton of
NullTimer()in DaCe-owned code. This PR moves the construction of theTimerup to calling code.How Has This Been Tested?
To be tested locally by running the
FVDynamicstranslate test in stencil mode and orchestrated. Both translate tests were still passing.Checklist: