load config files in order, later overrides earlier, lots more testing #108
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
This was
linked to
issues
Apr 3, 2024
|
why the merge commit? |
Default behavior in the github UI, clicked without thinking. I think we can configure this to default to rebase/ff. 
|
jaidhyani
force-pushed
the
config_file_order
branch
from
10c58e9 to
0cc0c98
Compare
|
https://github.com/orgs/community/discussions/12032 |
jettjaniak
approved these changes
Apr 3, 2024
There was a problem hiding this comment.
nice! 🚀
- why do we need
filter_config_to_actual_config_values? - in general I'm quite overwhelmed by train/config/utils.py, I didn't realize we'll need all of this to have the functionality I imagined
- could you add some docstrings? like what are the functions doing and why we need them
Comment on lines
57
to
+59
| "--config_files", | ||
| "--config_file", | ||
| "-c", |
There was a problem hiding this comment.
Suggested change
| "--config_files", | |
| "--config_file", | |
| "-c", | |
| "--config", | |
| "-c", |
I don't care about backward compatibility, no one is using this yet
I think we actually don't need this anymore, going to optimistically delete. |
I added some more docstrings explaining how some things are used, why we need them, and how they work. I also deleted a few things that we don't need anymore. |
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.
The main functional impact of this PR is to kill the "priority" config arg and just load configs in the order in which they're passed in to the training script, with the later overriding the former. Alongside this we do some refactoring to support testing to ensure that everything works as expected. We also improve type-casting override argument strings to better match the actual config types and make it significantly less dangerous.