-
Notifications
You must be signed in to change notification settings - Fork 767
Fix core cfg listed twice in report in default run #1544
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’ll 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?
Fix core cfg listed twice in report in default run #1544
Conversation
Reset config_files list at start of load_config() since it already includes garak.core.yaml in its settings_files and will reload everything. This prevents duplication when load_base_config() is called before load_config() in the CLI flow.
jmartin-tech
left a comment
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.
Clearing a global variable does not remove the impacts the previous file may have already had on state. This method does not clear all package configurable state before processing.
At this time _config package global values are treated as common state and the files loaded are stored in config_files. A solution to duplicate entries in config_files would need to account for this. I would suggest some set logic and guards to avoid duplicate processing of files may be needed.
Understood @jmartin-tech , updated to stop duplicate processing of config files instead |
leondz
left a comment
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.
lgtm - strange test failure though
|
Looking closer at the test failure, Lines 98 to 108 in 7849f3b
This fixture clears and reimports _config. This further exposes that design of the _config package needs some work if garak is used in an environment that will need to maintain state for multiple runs. The cli usage pattern masks this issue at this time.
|
|
#1549 addresses the global state issues that cause the test failure in this PR. Once that PR lands merging |
@jmartin-tech are we good to re-run the tests? I can't seem to do it on my end |
|
Re-running |
|
A commit is required to get tests to update to use the latest code, this can simply be a merge of |
Verification
Here's a verification checklist for this fix:
Verification
List the steps needed to make sure the duplicate config_files fix works