-
Notifications
You must be signed in to change notification settings - Fork 44
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
support advanced Garak configs #315
support advanced Garak configs #315
Conversation
@@ -54,5 +55,5 @@ repos: | |||
- --max-line-length=120 | |||
- --min-public-methods=0 | |||
- --good-names=o,w,q,f,fp,i,e | |||
- --disable=E0401,W1201,W1203,C0114,C0115,C0116,C0411,W0107,W0511,W0702,R0801,R1705,R1710 | |||
- --disable=E0401,W1201,W1203,C0114,C0115,C0116,C0411,W0107,W0511,W0702,R0801,R1705,R1710,W0201 |
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.
adding this for now, as the issue occurs again. This will need to be more investigated separately.
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.
I'm not completely sure about the configuration management approach here. The extra layer on top of the Garak config doesn’t seem to add much value and might actually limit advanced users. For example, if they want to tweak the 'reporting' section, they might not be able to. It could be simpler if we just let advanced users bring in their own config file
@ccronca Adding my two cents here. It would be good if we only have one configuration file so if advanced users want to modify something, maintaining different configuration files can be a mess. If they could do everything in the rapidast config file, that would be awesome. |
Good point! Having a single file makes total sense. My main concern is that it looks like Rapidast is duplicating the Garak config (at least the top-level stuff) in its own config. So, every time the Garak config changes, Rapidast has to be updated too. Since Rapidast isn’t really doing anything with that config, it might be better to just merge the default values from Rapidast with whatever the user provides and pass that to Garak. Garak can then check if the config is correct or not |
@ccronca Thank you for the response. That sounds like a valid approach! |
@ccronca Passing the garak config schema as is was actually one of options that I also considered so I'm okay with that approach too. The reason I ended up implementing it this way for now is that we may want to simplify the format as much as possible and add some schema checks for the top level at least, rather than just passing it to Garak ( later when we want to improve the config schema, the simplicity will matter more). How do the following three examples look to you? Example 1: (current)
VS Example 2 (just using Garak config as is)
I have also thought of supporting both by adding a key like 'garak_config' Example 3 (supporting both simplified format and the original garak config format. in this case, we need to decide which value of the same config option will be used if both exists )
Which one would you suggest? @jperezdealgaba what is your preference too from an end user perspective? |
IMHO, I would use number two so users can copy/paste it later and use it with Garak as a stand-alone |
I’d avoid having multiple ways to define the same config items, so option 3 is out. Between 1 and 2, I prefer the second one since it follows Garak’s existing structure, meaning we don’t need to map any config item, Rapidast can just pass the config as is. We could wrap all Garak-related configs under a parent key like "garak_config", so it's clear what belongs to Garak vs. any future Rapidast-specific settings. So, I’d go with option 2, but with everything under a key. |
It looks in my opinion with the new changes! |
* support full garak config under 'parameters'
(updated) Support Garak config format as is under the 'garak_config' key, according to the discussion
The changed config example looks like the following now.