16 fix config file behavior#19
Open
yonatank93 wants to merge 17 commits into
Open
Conversation
…ult values In this version, the keys and values in the configuration file are treated as keyword arguments to instantiate classes. This makes the behavior of running with configuration file/factory the same as if we instantiate the classes manually. For the environment class, the dataset argument is replaced with dataset_path, so that one can pass in file without needing to load the file first. Special treatment is for the configurations inside AutoREC/default_configs, where the dataset_path there is treated as path relative to the configuration file instead of to the current directory. This is because those default configurations are used for example/testing.
…nt treatment of dataset_path
…efault_configs I also created a new default configs for the docker run demo. Instead on adding more function in factory to also handle this case, I think it is just easier to just have an additional config file. It is not big anyway.
… run with minimum argument
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the configuration/factory workflow so YAML/dict configs are treated purely as constructor keyword arguments (no merging with packaged defaults), and makes dataset_path required to align factory behavior with manual instantiation.
Changes:
- Remove default-config fallback/merging behavior; require
dataset_pathand load datasets explicitly in the factory. - Add special-case path handling so
example/default_configs/*resolvedataset_pathrelative to the config file (and/app/default_configsinside Docker). - Reorganize/move example configs and update docs + Docker build inputs accordingly.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/autorec/factory.py |
Removes default merging, requires dataset_path, adds _config_reader for example-config path resolution. |
src/autorec/cli.py |
Switches config loading to _config_reader for the new example-config path behavior. |
README.md |
Updates configuration documentation and example command paths to example/default_configs. |
Dockerfile |
Copies example/default_configs into the container at /app/default_configs. |
pyproject.toml |
Removes packaging of src/autorec/default_configs/*.yaml. |
example/main_factory.py |
Adds CLI args and adjusts output-dir handling to use agent.save_dir. |
example/hyperparameter_tuning/main_factory.py |
Updates default config path. |
example/default_configs/*.yaml |
Updates dataset paths and moves agent config into examples. |
src/autorec/default_configs/agent_config.yaml |
Deleted (defaults no longer loaded/packaged). |
default_configs/environment_config.yaml |
Deleted (example configs relocated). |
Comments suppressed due to low confidence (1)
src/autorec/factory.py:101
- When args is a dict, this function assigns
config = argsand then mutates nested structures (e.g., agent_config) by inserting environments. This leaks side effects to callers (including the CLI) and makes it harder to reuse the config dict after calling the builder.
if isinstance(args, (str, Path)):
config = _config_reader(args)
elif isinstance(args, dict):
config = args
else:
raise TypeError("The 'args' parameter must be a str, Path, or dict.")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Update the error message Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…oREC into 16_fix_config_file_behavior
Copilot suggestion for potential bug related to setting the save_dir in the agent configuration yaml file. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Update printed message. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Since there are already some data included, like pretrained model, dataset, etc., I think doing this reorganization makes it more consistent structure wise with the others.
Collaborator
Author
|
@ajaberi1 I think this is ready for you review. After this is merged, then I will work on unit tests. |
29 tasks
This file contains hidden or 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
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.
Summary
Fix the behavior of the factory (both reading from a YAML file and instantiating classes from a dictionary)
Related issue
Link the issue that this pull request addresses.
Type of change
Please check all that apply:
Motivation and context
We want the same behavior whether we instantiate the classes manually or using factory for consistency.
Additional dependencies introduced, if any
Not applied
TODO, if any
Not applied
Checklist
Before this pull request is merged, please check the following:
Additional comments
Not applied