arch: rm support for init'ing attempt.prompt with str
#1361
Conversation
|
consider removing support for |
| message = entity.pop("content", {}) | ||
| if isinstance(message, str): | ||
| content = Message(text=message) | ||
| content = Message(message, str) |
There was a problem hiding this comment.
I don't think this revision is correct, str is a class and passing it as the second argument to this constructor would not be valid. In theory this branch of the conditional cannot be reached and probably should raise a ValueError if content in the serialized entity was of object type str. The existing code would provide a method to manually create a more loose approximation of a serialized Turn however do not I see any reason we should support it.
There was a problem hiding this comment.
This doesn't look right either, and I suspect something like this should have been flagged in tests. Will check tests.
There was a problem hiding this comment.
Yes, in theory this code cannot actually be reached as we only deserialize Message objects that have been serialized from a valid original Message. I suspect existing tests never enter this conditional and if can be removed or adjusted to raise ValueError.
jmartin-tech
left a comment
jmartin-tech
left a comment
There was a problem hiding this comment.
Early review thoughts.
| elif isinstance(prompt, str): | ||
| msg = Message(text=prompt, lang=lang) | ||
| raise ValueError( | ||
| "attempt Prompt must be Message or Conversation, not string" | ||
| ) | ||
| # msg = Message(text=prompt, lang=lang) |
There was a problem hiding this comment.
Prefer to just remove this.
There was a problem hiding this comment.
OK, will leave this in until general functionality looks like it's in place. Wouldn't mind gating on a debug flag - but we don't have one, and this affects control flow, so gating on verbosity isn't the way
| if isinstance(value, str): | ||
| # note this does not contain a lang | ||
| self._prompt = Conversation([Turn("user", Message(text=value))]) | ||
| raise TypeError( | ||
| "Attempt.prompt must be Message or Conversation, not bare string" | ||
| ) | ||
| # self._prompt = Conversation([Turn("user", Message(text=value))]) |
There was a problem hiding this comment.
Again prefer to just remove this.
| d = _plugins.load_plugin("detectors.continuation.Continuation") | ||
|
|
||
| a = garak.attempt.Attempt(prompt="test prompts") | ||
| a = garak.attempt.Attempt(prompt=garak.attempt.Message(text="test prompts")) |
There was a problem hiding this comment.
Many of the edits in this file look ripe for a single CONSTANT.
| d = MitigationBypass() | ||
| attempt = garak.attempt.Attempt(prompt="testing prompt", lang=d.lang_spec) | ||
| attempt = garak.attempt.Attempt( | ||
| prompt=garak.attempt.Message(text="testing prompt"), lang=d.lang_spec |
There was a problem hiding this comment.
While I don't think we actually have any detectors with multiple lang values in lang_spec this should probably be explicit.
| prompt=garak.attempt.Message(text="testing prompt"), lang=d.lang_spec | |
| prompt=garak.attempt.Message(text="testing prompt"), lang=d.lang_spec.split(",")[0] |
There was a problem hiding this comment.
OK, so plugin .lang_spec is a spec and lang (in e.g. Message) is a single choice. Which is clear from var name. Let's aim to continue with this explicit & clear convention.
2 participants
fixes #1347
attempt.promptacceptsMessageorConversation- now we check for thishey seeing as local models are difficult to run, are perhaps the minority case, and are where most of our deps come from, what are thoughts on dropping these, and migrating to something strongly typed, like rust?
Verification