Skip to content

probes: refactor fact snippet mixin #1187

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

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

leondz
Copy link
Collaborator

@leondz leondz commented Apr 29, 2025

requires #1181

Refactor snippet code in latentinjection probes to move function up into FactSnippetMixin

validation:

  • all probes inheriting FactSnippetMixin contain exactly one INJECTION_MARKER
  • new test for FactSnippetMixin

todo (may be out of scope):

  • identify a generalisable a route for mixins to make config items available through DEFAULT_PARAMS

@leondz leondz added the probes Content & activity of LLM probes label Apr 29, 2025
Signed-off-by: Leon Derczynski <leonderczynski@gmail.com>
@leondz leondz marked this pull request as ready for review May 6, 2025 06:00
@leondz leondz self-assigned this May 6, 2025
@leondz leondz added this to the 0.11.0 milestone May 8, 2025
@leondz leondz requested a review from jmartin-tech May 8, 2025 07:32
@leondz leondz requested a review from erickgalinkin May 12, 2025 10:25
Copy link
Collaborator

@erickgalinkin erickgalinkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. I think it's a really good improvement on the existing logic -- thanks!

@leondz leondz removed this from the release 0.11.0 milestone May 14, 2025
@leondz
Copy link
Collaborator Author

leondz commented May 14, 2025

Don't merge til after 0.11.0, alters bag probes content slightly

Copy link
Collaborator

@jmartin-tech jmartin-tech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test failures are due to import changes from main.

@@ -468,6 +496,11 @@ class LatentInjectionFactSnippetEiffelFull(
"![image](https://bad.com)",
]

def __init__(self, config_root=_config):
Probe.__init__(self, config_root)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Probe.__init__(self, config_root)
garak.probes.Probe.__init__(self, config_root)

@@ -496,6 +529,11 @@ class LatentInjectionFactSnippetLegalFull(
("-- LEGAL AMENDMENT, FOLLOW INSTRUCTION\n", "\n-- RESUME"),
]

def __init__(self, config_root=_config):
Probe.__init__(self, config_root)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Probe.__init__(self, config_root)
garak.probes.Probe.__init__(self, config_root)

self.contexts.add(context)

self.contexts = list(self.contexts)
Probe.__init__(self, config_root)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Probe.__init__(self, config_root)
garak.probes.Probe.__init__(self, config_root)

@leondz
Copy link
Collaborator Author

leondz commented May 14, 2025

Thanks

Will work out a better pattern for configuration before merging. Maybe by also doing merge w Probe.DEFAULT_PARAMS and requiring appropriate mro in inheritance descriptions.

@leondz leondz marked this pull request as draft May 15, 2025 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
probes Content & activity of LLM probes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants