-
Notifications
You must be signed in to change notification settings - Fork 41
Check input model hypers when restarting trainings #1232
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
5bcb6e1
2874fa9
be37440
3586e31
94cd929
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| from typing import Type, TypedDict, TypeVar | ||
| from typing import Any, Type, TypedDict, TypeVar | ||
|
|
||
| from typing_extensions import TypedDict as TE_TypedDict | ||
|
|
||
|
|
@@ -97,3 +97,60 @@ def overwrite_defaults( | |
| :param new_defaults: A dict with the new default hyperparameters. | ||
| """ | ||
| _OVERWRITTEN_DEFAULTS[hypers_cls] = new_defaults | ||
|
|
||
|
|
||
| def get_hypers_diff( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't there a function already from the libraries that we use?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I will check Omegaconf, this was just so that it was clear what I propose to do
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| old_hypers: dict, | ||
| new_hypers: dict, | ||
| ) -> dict[str, tuple[Any, Any]]: | ||
| """Get the difference between two hypers dictionaries. | ||
|
|
||
| :param old_hypers: The old hyperparameters. | ||
| :param new_hypers: The new hyperparameters. | ||
|
|
||
| :return: A dict with the hyperparameters that are different in the new | ||
| hypers compared to the old hypers. It is assumed that every key in | ||
| the new hypers is also present in the old hypers. | ||
| """ | ||
| diff = {} | ||
| for key, new_value in new_hypers.items(): | ||
| old_value = old_hypers[key] | ||
| if old_value != new_value: | ||
| diff[key] = (old_value, new_value) | ||
|
Comment on lines
+121
to
+132
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does a single loop work if there are deeply nested dictionaries?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok I think this answers my other question: order doesn't matter, right? |
||
| return diff | ||
|
|
||
|
|
||
| def raise_hypers_mismatch( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. I would think there is already something. But maybe I am wrong. |
||
| hypers_diff: dict[str, tuple[Any, Any]], | ||
| ) -> None: | ||
| """Raise an error if the hypers diff is not empty. | ||
|
|
||
| The hypers diff can be computed using :func:`get_hypers_diff`. | ||
| """ | ||
| if hypers_diff: | ||
| n_mismatches = len(hypers_diff) | ||
| raise ValueError( | ||
| f"Found {n_mismatches} mismatch{(n_mismatches != 1) * 'es'} in model hyperparameters.\n" | ||
| f"Mismatched hypers: {list(hypers_diff.keys())}\n" | ||
| "\n-------- Mismatches --------\n\n" | ||
| + "\n".join( | ||
| f"[Mismatch {i + 1}] {key}\n Previous: {old}\n New: {new}" | ||
| for i, (key, (old, new)) in enumerate(hypers_diff.items()) | ||
| ) | ||
| ) | ||
|
|
||
|
|
||
| def raise_if_hypers_mismatch( | ||
| old_hypers: dict, | ||
| new_hypers: dict, | ||
| ) -> None: | ||
| """Raise an error if the new hypers do not match the old hypers. | ||
|
|
||
| :param old_hypers: The old hyperparameters. | ||
| :param new_hypers: The new hyperparameters. | ||
| """ | ||
| # Gather mismatchs | ||
| mismatches = get_hypers_diff(old_hypers, new_hypers) | ||
|
|
||
| if mismatches: | ||
| raise_hypers_mismatch(mismatches) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think this this is the canoncial place! Very good.