Added lru caches to speed up validphys multi-replica initalization #1945
Conversation
| @@ -1247,6 +1249,8 @@ def parse_default_filter_rules_recorded_spec_(self, spec): | |||
| def parse_added_filter_rules(self, rules: (list, type(None)) = None): | |||
| return rules | |||
|
|
|||
| @freeze_args | |||
| @functools.lru_cache | |||
There was a problem hiding this comment.
a note to myself to check what enters here and whether maybe the problems are related to what I see in #1678 (since if so, that won't be a problem)
Are you absolutely certain this one is needed for the multi-replica? Since in principle this function should only happen once.
There was a problem hiding this comment.
I will check in master
There was a problem hiding this comment.
For NNPDF4.0 with 3 replicas it is called 1001 times. For 2 replicas 693 times.
There was a problem hiding this comment.
I see. Then indeed, let's keep the freeze args for now, but please add a comment with that info on top. That should definitely not happen since the rules should be the same per replica, so I believe this is a signal of a problem somewhere else which hopefully can eventually be solved.
There was a problem hiding this comment.
Added comment in 19771a0
| @@ -561,6 +563,8 @@ def _make_point_namespace(self, dataset, idat) -> dict: | |||
| return ns | |||
|
|
|||
|
|
|||
| @freeze_args | |||
| @functools.lru_cache | |||
There was a problem hiding this comment.
Same note to myself.
But this one I think might be solved by some of the stuff I'm doing in the reader.
There was a problem hiding this comment.
For NNPDF4.0 with 3 replicas it is called 1001 times. For 2 replicas 693 times.
--> I'm sorry, this comment was intended for the other function produce_rules in config.py!!!
There was a problem hiding this comment.
This one yes, but I think it might be fixed now (in other PR). The other one I'm more worried about.
There was a problem hiding this comment.
For NNPDF4.0 with 3 replicas it is called 1040 times. For 2 replicas 732 times.
|
Testing with NNPDF4.0 runcard (theorid 200) with 200 epochs and 3 replicas on my laptop yields no numerical differences with the master branch (16 digits) |
validphys2/src/validphys/utils.py
Outdated
| return False | ||
|
|
||
|
|
||
| def immute(obj: Any): |
There was a problem hiding this comment.
| def immute(obj: Any): | |
| def make_hashable(obj: Any): |
For symmetry reasongs, since you have is_hashable
validphys2/src/validphys/utils.py
Outdated
| # Since typing.Hashable doesn't check recursively you actually | ||
| # have to try hashing it. |
There was a problem hiding this comment.
This I don't really understand, what cases would we miss by doing isinstance(obj,Hashable)?
There was a problem hiding this comment.
I agree, and I prefer the type check you propose. Will implement it
There was a problem hiding this comment.
It works just as well, see 19771a0
APJansen
force-pushed
the
lru-cache-validphys
branch
from
264efaa to
61e5953
Compare
|
What is the status of this? Anything still needed? (Just rebased to master) |
|
For me it is ok. As I said, I hope to be able to remove the |
Co-authored-by: Juan M. Cruz-Martinez <juacrumar@lairen.eu>
Co-authored-by: Roy Stegeman <roystegeman@live.nl>
Co-authored-by: Roy Stegeman <roystegeman@live.nl>
APJansen
force-pushed
the
lru-cache-validphys
branch
from
a35879d to
dd4b98a
Compare
Added lru caches in validphys to accelerate multi-replica initialization