Add a look back constraint on spiral errors #1205
Conversation
guillett
force-pushed
the
spiral-management
branch
from
d641829
to
76a1cb2
Compare
guillett
force-pushed
the
spiral-management
branch
from
50b450a
to
d9c8aec
Compare
guillett
force-pushed
the
spiral-management
branch
from
d9c8aec
to
ab478d6
Compare
| count += 1 | ||
| if count > self.max_spiral_loops: | ||
| break | ||
| invalidate_entries = False |
There was a problem hiding this comment.
I notice that this delta removes a comment. The comment noted that the stack was visited in reverse order; I'd assume that was a significant decision. I'm a bit worried to see this delta use a different order AND remove the comment.
"The tests still pass, so it must be OK" is an objection that I could understand, but perhaps in the context of OF-Core it's not as convincing as it would be in other contexts…
There was a problem hiding this comment.
I realized that the intention could be improved incrementally. I update the function code. In my opinion, it is a much clearer version.
| @@ -48,6 +48,8 @@ def __init__( | |||
|
|
|||
| # controls the spirals detection; check for performance impact if > 1 | |||
| self.max_spiral_loops: int = 1 | |||
| self.max_spiral_lookback_months: int = 0 | |||
There was a problem hiding this comment.
What happens with days, weeks, years? Shouldn't we also cover for these? I'm still trying to grasp the desired behaviour in terms of the domain logic.
There was a problem hiding this comment.
Thanks a lot @guillett!
This looks like a very interesting proposal. If I'm getting it right this represents a major advance in spirals logic and therefore also a breaking change.
In order to accept this pull request, as a reviewer, it will help to have a better description of how this changes the current domain logic and, for impacted users, how can they migrate in case of experiencing errors after the upgrade. Also, given it is –it seems– a breaking change, I suggest you add the corresponding new tests.
In particular, to the months lookback logic.
| spiral = len(previous_periods) >= self.max_spiral_loops | ||
| if spiral: | ||
|
|
||
| too_many_spirals = len(previous_periods) >= self.max_spiral_loops |
There was a problem hiding this comment.
May I propose to give it another name and to extract it to a method? As it is a core domain rule that we're introducing here, it makes sense to me to be able to test it in isolation. Just an idea:
def spiral_loops_within_threshold(previous_periods: list[Period | str]) -> bool:
return len(previous_periods) < self.max_spiral_loops| if spiral: | ||
|
|
||
| too_many_spirals = len(previous_periods) >= self.max_spiral_loops | ||
| too_backward = (previous_periods[0].date - period.date).in_months() > self.max_spiral_lookback_months if previous_periods and self.max_spiral_lookback_months > 0 else False |
There was a problem hiding this comment.
I don't understand how this new domain rule relates to the previous one. Is it a specific check just for months that overrides the previous one? If it is, it makes all sense for me to allow contry package developers set thresholds for the period types that concern them independently (weeks, months, years...).
Beyond that, it would be great, as with the previous one, to extract it to a method to give it more visibility.
| @@ -412,17 +416,9 @@ def invalidate_cache_entry(self, variable: str, period): | |||
| self.invalidated_caches.add(Cache(variable, period)) | |||
|
|
|||
| def invalidate_spiral_variables(self, variable: str): | |||
| # Visit the stack, from the bottom (most recent) up; we know that we'll find | |||
There was a problem hiding this comment.
It would be great if you could instead update the documentation.
| count = 0 | ||
| for frame in reversed(self.tracer.stack): | ||
| first_variable_occurrence = next(index for (index, frame) in enumerate(self.tracer.stack) if frame["name"] == variable) | ||
| for frame in self.tracer.stack[first_variable_occurrence:]: |
There was a problem hiding this comment.
I didn't quite get what this line changes compared to the previous one. Is it just a refactoring or the underlying logic is changed as well? (As this is already tested, I presume there is no change in logic here specifically. However, as you introduce a new constraint a couple of lines above, I'm not 100% sure).
bonjourmauko
added
level:advanced
Depends on #1206
New features