Check input model hypers when restarting trainings - #1232
Conversation
| return self.outputs | ||
|
|
||
| def restart(self, dataset_info: DatasetInfo) -> "PET": | ||
| def restart( |
There was a problem hiding this comment.
Yes, I think this this is the canoncial place! Very good.
| return diff | ||
|
|
||
|
|
||
| def raise_hypers_mismatch( |
There was a problem hiding this comment.
Same here. I would think there is already something. But maybe I am wrong.
|
the overall design looks good to me! |
2874fa9 to
6f2c41b
Compare
|
Ok this is ready for a final review |
6f2c41b to
be37440
Compare
jwa7
left a comment
jwa7
left a comment
There was a problem hiding this comment.
I think this looks good and I like what it enables, but I want to question how restrictive this is. For instance one of the current CI failure looks like:
[Mismatch 1] soap
Previous: {'max_angular': 2, 'max_radial': 4, 'cutoff': {'radius': 5.0, 'width': 0.5}}
New: {'max_radial': 4, 'max_angular': 2}
and I'm wondering if this is what we want - are implied default hypers no longer acceptable?
| # This should work, as we are not changing the hypers | ||
| model.restart(dataset_info=dataset_info) | ||
| model.restart(dataset_info=dataset_info, model_hypers={}) | ||
| model.restart(dataset_info=dataset_info, model_hypers=minimal_model_hypers) |
There was a problem hiding this comment.
How about same hypers but in a different order?
There was a problem hiding this comment.
We can add the check just in case
|
Yes that one should work, but that's good this is why we have tests 😄 |
|
However it's not obvious what should be the behavior there. People would expect that whatever is not specified in the yaml is the defaults or the current hypers of the model? |
|
Maybe we can start by covering only the unambiguous case:
In principle this would be for nested keys, since for top-level ones we are assuming the missing keys are the current model hypers. But we could also be more strict there if you think it's not fine? |
|
I was planning to do something different @HaoZeke , so I will revert. In any case, I think pushing into branches of other people without asking first or having been asked for a review is not the way to go 😅 |
|
Ah well you were asked for a review, but that is just the automatic one for being the maintainer of dpa3 haha |
|
oops, my bad |
0078884 to
7b537ae
Compare
|
@PicoCentauri I implemented dealing with nested keys as I proposed here: #1232 (comment) Could you check if that is ok? Then I will add this to the changelog and possibly to the docs and we could merge 👍 |
| if default_hypers is not None: | ||
| hypers_keys = list(new_hypers.keys()) | ||
| merged = OmegaConf.merge( | ||
| OmegaConf.create(default_hypers), OmegaConf.create(new_hypers) | ||
| ) | ||
| merged = OmegaConf.to_container(merged) | ||
| new_hypers = {k: merged[k] for k in hypers_keys} |
There was a problem hiding this comment.
@PicoCentauri this is mainly the new change in the logic
2b5ef8b to
896c9bc
Compare
5 participants
This is a first design of something that was discussed today in the devel meeting.
When restarting, it should be checked that if the user has provided model hypers in the input yaml, those match the hypers present in the checkpoint.
This enables the possibility that in the future, architectures will allow not only perfect matches, but also updating the model to some compatible hypers. This is for example the case in #1209 , which is what spawned the discussion leading to this PR.
@Luthaf @PicoCentauri could you check if something like this would work?
📚 Documentation preview 📚: https://metatrain--1232.org.readthedocs.build/en/1232/