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.
| _OVERWRITTEN_DEFAULTS[hypers_cls] = new_defaults | ||
|
|
||
|
|
||
| def get_hypers_diff( |
There was a problem hiding this comment.
Isn't there a function already from the libraries that we use?
There was a problem hiding this comment.
Yes I will check Omegaconf, this was just so that it was clear what I propose to do
There was a problem hiding this comment.
Ok, so apparently neither omegaconf or pydantic have such a functionality, and since we are just checking a shallow diff I think it is fine to have this simple function here
| 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.
| for key, new_value in new_hypers.items(): | ||
| old_value = old_hypers[key] | ||
| if old_value != new_value: | ||
| diff[key] = (old_value, new_value) |
There was a problem hiding this comment.
Does a single loop work if there are deeply nested dictionaries?
There was a problem hiding this comment.
Depends on what you mean by "work". It will detect that the dictionaries are different if there is a difference somewhere deep in the keys. But it will include still all the deep keys that are the same. My idea was that dealing with arbitrary nested keys is a bit of a mess and perhaps we can introduce it whenever some model needs it? For example, for the user the top-level key that changed is likely enough to report in the error.
There was a problem hiding this comment.
Ok I think this answers my other question: order doesn't matter, right?
|
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
| for key, new_value in new_hypers.items(): | ||
| old_value = old_hypers[key] | ||
| if old_value != new_value: | ||
| diff[key] = (old_value, new_value) |
There was a problem hiding this comment.
Ok I think this answers my other question: order doesn't matter, right?
|
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? |
A restart yaml that omits soap.cutoff defaults was treated as a mismatch against the full checkpoint dict. Recurse into mappings and treat missing nested keys as keep-the-checkpoint, matching top-level.
|
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 |
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/