arch: re-work generator name param #1453
#1456
Conversation
Signed-off-by: Noah <[email protected]>
There was a problem hiding this comment.
While the spirit of this PR makes sense, the ask in #1453 may be a bit unclear.
It is already possible to instantiate all plugins without passing name as a named parameter. The documented precedence of configuration is constructor named parameter > config_root provided attribute > DEFAULT_PARAMS value.
As such the current changes here are unnecessary as the existing code should already respect this precedence order. I will note I see a few locations where the changes in the PR do not retain the precedence order.
The current constructor signature however for most generators accepts name as a direct named parameter and the goal would be to remove this entry. name was previously expected to be a required parameter for all Generator plugins however with the shift to enable more generic Configurable attributes this explicit parameter can be deferred to a config_root key reducing the precedence chain.
The issue was opened specifically because the removal of the constructor named parameter would be a breaking change for any consuming code that attempts to instantiate a generator directly will need to be thoroughly tested as it would impact signature of all Generator plugins.
One concern about name generators is that this parameter is utilized for reporting details and any refactor needs to ensure to account for this.
|
|
||
| # Ensure suppressed_params is a set for efficient lookup | ||
| self.suppressed_params = set(self.suppressed_params) | ||
|
|
There was a problem hiding this comment.
Changes should not remove code not related to the issue.
| raise e | ||
|
|
||
| super().__init__(self.name, config_root=config_root) | ||
| super().__init__(uri, config_root=config_root) |
There was a problem hiding this comment.
Multiple earlier locations already validated what self.name should be. This revision would change expected behavior.
leondz
changed the title
name param #1453
2 participants
Changes
Fixed 12 generator files:
garak/generators/guardrails.py
garak/generators/nvcf.py
garak/generators/nemo.py
garak/generators/langchain.py
garak/generators/huggingface.py (3 classes: Pipeline, InferenceAPI, LLaVA)
garak/generators/openai.py (2 classes: OpenAICompatible, OpenAIGenerator)
garak/generators/ggml.py
garak/generators/cohere.py
garak/generators/litellm.py
garak/generators/function.py
garak/generators/rest.py (changed to pass uri parameter)
Testing
All 39 generator structure tests pass
All 103 generator tests pass (2 failures are unrelated API auth errors)
Verified generators can be instantiated via load_plugin with config-only name
Benefits
Generators can be instantiated using load_plugin and the Configurable pattern alone
name is not hardcoded outside of construction
name is not duplicated (set once after config loads)
name is optional (can come from config without a constructor parameter)