Extend functionality of Wandb Config Diff script #687
+79
−25
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nt compare wandb config script to also flatten list dicts
dirkgr
requested changes
Aug 2, 2024
olmo/util.py
Outdated
| new_list.append( | ||
| flatten_dict( | ||
| v, | ||
| parent_key=root, |
There was a problem hiding this comment.
The normal way of doing this is actually to treat a List as a Mapping[int, Any]. So the key becomes something like "foo.bar.0" and "foo.bar.1", etc. Then you don't need the extra root parameter either.
| } | ||
| if len(keys_with_differences) > 0: | ||
| for k in sorted(keys_with_differences): | ||
| if isinstance(left_config[k], list) and isinstance(right_config[k], list): |
There was a problem hiding this comment.
You also don't need this if you treat lists as Mapping[int, Any]. And it will work right even if the list entries are complex. On the other hand, the output will look different / be less compact.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
flatten_dict()in utilsflatten_dict()to also flatten any dicts that exist in Listsflatten_dict()Motivation is, while comparing configs, the current implementation doesn't perform comparison of some key aspects of the configs, namely config keys representing dataset paths (which are all List[str]) as well as keys like
config["evaluators.value"]which areList[Dict].The current behavior looks something like this:
where we can see that the fields
data.value.pathsandevaluators.valuearen't easily comparable.The new behavior looks like this:
where it preserves behavior of original script under old keys, but performs side by side comparison of list elements also.
The downside, of course, is with a lot of dataset paths, these config diffs can become quite long to sift through.