Feature: configurable plugins

jmartin-tech commented 4 months ago

Fix #589 Fix #595

Buckle up lots of adjustments to review. Many of the generator/probe/detector/buff/harness plugins are refactored in some way or another.

The goal of this change is to offer configuration of instance variables on plugins on a per "type" basis.

Summary of changes:

all plugins implementing Configurable and accept a named config_root parameter that defaults to the global _config
shift all configurable class variables into DEFAULT_PARAMS to avoid leaking instance specific values
inject all default params as instance attributes
provide exmaple for _supported_params in class definitions
propagate keys in the plugin yaml dictionalry as attributed on plugin instantiation
mechanism based on ENV_VAR class constant to obtain api_key during instantiation
plugin provides defining a class level _supported_params only apply supported params from configuration files, a warning is logged for unknown items
refactor inline configuration yaml refactored for readability
rasa.RasaRestGenerator is implements an extension of rest.RestGenerator with embedded defaults and updated module config docs to increase code reuse
garak.generators.load_generator() and other generator loading code is removed in favor of _plugin.load_plugin() accounting for DEFAULT_CLASS
remove plugin configuration method in favor of Configurable

Configuration files apply in a hierarchal fashion and allow configuration of parameters not exposed in the class initializer.

Final values for configured attributes as instance variables are applied in with precedence to programatic input with a nuance that a constructor default value can be overwritten by a configuration file value if the provided value is the same as the default.

value from named parameter in constructor 
  else: 
    value from config passed to constructor
    else:
      default provided or inherited in class

The changes introduce breaking changes to the expected structure of configuration wether provided as yaml or json. The primary breaking change is the expectation that configuration of a class will be nested inside configuration for all classes of a module holding more specific resolutions as the maintained values. This format change is currently left in a partially compatible state to provide time for consumers to update configuration formats. Here is an example of a how configuration of the rest.Generator class is expected to change.

        {"rest.RestGenerator":
            {
                "name": "example service",
                "uri": "https://example.ai/llm",
                "method": "post",
                "headers":{
                    "X-Authorization": "$KEY",
                },
                "req_template_json_object":{
                    "text":"$INPUT"
                },
                "response_json": true,
                "response_json_field": "text"
            }
        }

Becomes:

      {
          "rest": {
              "RestGenerator": {
                  "name": "example service",
                  "uri": "https://example.ai/llm",
                  "method": "post",
                  "headers": {
                      "X-Authorization": "$KEY",
                  },
                  "req_template_json_object": {
                      "text": "$INPUT"
                  },
                  "response_json": true,
                  "response_json_field": "text"
              }
          }
      }

If provided the previous format a warning is logged in garak.log about used of deprecated formatting.

leondz commented 4 months ago

OK, I can definitely see that uri on its own is always ambiguous, we can shift that other use to doc_uri. Leaving it as a class attribute (immutable) seems like it might be enough signal re: not updating it?

On Fri, May 31, 2024, 18:03 Jeffrey Martin @.***> wrote:

@.**** commented on this pull request.

In garak/generators/langchain_serve.py https://github.com/leondz/garak/pull/711#discussion_r1622632922:

if self.uri is None:

self.uri = os.getenv("LANGCHAIN_SERVE_URI")

if not self._validate_uri(self.uri):

I see what you are referring to, at the class level uri is being reserved in some plugins.

I land the other direction, I would say the uri attribute should not be reserved for "references" and it would be more clear to define specific documentation in an attribute with a lower probability for collision and even possibly as a list, something like references, public_sources, of maybe public_references. Maybe even use a constant like REFERENCES to denote that the value is not expected to ever be manipulated in by an instance.

— Reply to this email directly, view it on GitHub https://github.com/leondz/garak/pull/711#discussion_r1622632922, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA5YTS6CDCRJFJU6J4ZUWLZFCNL5AVCNFSM6AAAAABIR4IWXKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAOJRGE4TONZQGQ . You are receiving this because your review was requested.Message ID: @.***>

jmartin-tech commented 4 months ago

Thanks for all the feedback, I will work thru it all and post updated code soon.

leondz / garak

Feature: configurable plugins #711

@.**** commented on this pull request.