Skip to content
This repository has been archived by the owner on Oct 19, 2024. It is now read-only.

Add possibility to override configs #7

Merged
merged 10 commits into from
Nov 4, 2023

Conversation

jonasteuwen
Copy link
Contributor

This commit implements a method to use the additional_config folder to override specific configurations.

@BPdeRooij
Copy link
Contributor

BPdeRooij commented Oct 23, 2023

The additional_config functionality works, but this is already a feature of Hydra (Docs). Adding ../additional_config to train.yaml as a Hydra searchpath achieves the same functionality, without cluttering the code with an unneeded plugin.
When hardcoding the location of additional_config, it might be good to add both an __init__.py and a .gitkeep.

@jonasteuwen
Copy link
Contributor Author

I didn't get it to work the way I wanted (in the order that it will be overwritten). Did you manage to keep the functionality as mentioned? I think the plugin might still be interesting, as we can also extend it to read directly from repositories.

@BPdeRooij
Copy link
Contributor

It seems that this plugin would still be interesting with the possibility to extend it to read directly from repositories.

I have two comments regarding the expected behaviour of this PR:

  1. In manipulate_search_path no warning is given if the additional_path is not a directory. Perhaps logging something something simple, such as "Only using standard ahcore config folder for Hydra configurations.", would be good to avoid any confusion.
  2. The env.load_env function starts searching recursively from ahcore/utils folder instead of the working directory. It will find the .env folder if it is in a parent folder but not when placed in the tools directory.

- Be more verbose in additional_configs.py
@jonasteuwen
Copy link
Contributor Author

@BPdeRooij I believe I have addressed both concerns.

@BPdeRooij
Copy link
Contributor

dotenv.load_dotenv expects an explicit file name. It does not start searching from working_dir. For this we could use dotenv.find_dotenv(usecwd=True) and give this as dotenv_path

@jonasteuwen
Copy link
Contributor Author

jonasteuwen commented Oct 30, 2023 via email

Copy link
Contributor

@BPdeRooij BPdeRooij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@jonasteuwen jonasteuwen merged commit 30f3ecf into main Nov 4, 2023
@jonasteuwen jonasteuwen deleted the feature/add-additional-configs branch November 4, 2023 18:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants