-
Notifications
You must be signed in to change notification settings - Fork 248
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
generator: promote OpenAICompatible as first class generator #1021
base: main
Are you sure you want to change the base?
generator: promote OpenAICompatible as first class generator #1021
Conversation
Various generators extend this base template, usage and feedback suggest UX and usability improvements will be gained by promoting this class to be a `Configurable` generic OpenAI client based generator. * default `uri` * default implmentations of `_load_client()` and `_clear_client()` * remove duplicate versions of `_clear_client()` Signed-off-by: Jeffrey Martin <[email protected]>
f470652
to
5c56469
Compare
Signed-off-by: Jeffrey Martin <[email protected]>
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.
let's update class naming. maybe i didn't get the uri
param locking bit, either
# remove uri as it is not overridable in this class. | ||
DEFAULT_PARAMS = { | ||
k: val for k, val in OpenAICompatible.DEFAULT_PARAMS.items() if k != "uri" | ||
} | ||
|
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.
this is confusing given the triggering use-case. we should distinguish between generator for OpenAI API Compatible endpoints, and OpenAI Native endpoints hosted by OpenAI - recommend two distinct classes; perhaps one called openai.Compatible, and another called openai.Native, which is the default generator in the module, that implements this uri
locking
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.
Not sure if class name change would make it any clearer, as is:
openai.OpenAICompatible == openai.Compatible
openai.OpenAIGenerator == openai.Native
openai.OpenAIReasoningGenerator == openai.NativeReasoning
The default is still openai.OpenAIGenerator
where the uri
is not exposed as a default param as it would be ignored and technically locked
although it could still be provided as an env variable OPENAI_BASE_URL
, we could also add a check during _load_client()
to fail or warn if uri
was provided when it will be ignored.
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 think openai.Compatible
makes sense but I'd suggest leaving openai.OpenAIGenerator
as-is. We could also factor OpenAICompatible
out to another class (maybe to the base class?) in lieu of keeping it in the openai
generator spec.
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.
ah, and then openai.OpenAIReasoning
for the o1 stuff? OK
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.
Do we want to support base.OpenAICompatible
as a valid --model_type
? I land on a "no" there.
@erickgalinkin, are you thinking we should push the class down to base.py
and then have openai.Compatible
just expose the class, this would come with a requirement for import of openai
and backoff
in base.py
:
from garak.generators.base import OpenAICompatible
class Compatible(OpenAICompatible):
ENV_VAR = "OpenAICompatible_API_KEY".upper()
active = True
supports_multiple_generations = True
generator_family_name = "OpenAICompatible"
Depending on the answer above, this seems like a discussion focused on needing a standard in naming expectations for generator classes. There is currently a mix of including the model_type
value in the classname or not examples:
generators: litellm 🌟
generators: litellm.LiteLLMGenerator
generators: nim 🌟
generators: nim.NVOpenAIChat
generators: nim.NVOpenAICompletion
generators: nim.Vision
generators: huggingface 🌟
generators: huggingface.ConversationalPipeline
generators: huggingface.InferenceAPI
generators: huggingface.InferenceEndpoint
generators: huggingface.LLaVA
generators: huggingface.Model
generators: huggingface.OptimumPipeline
generators: huggingface.Pipeline
generators: octo 🌟
generators: octo.InferenceEndpoint
generators: octo.OctoGenerator
generators: function 🌟
generators: function.Multiple
generators: function.Single
I don't see any consistency to say what should be the preferred convention.
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.
Do we want to support
base.OpenAICompatible
as a valid--model_type
? I land on a "no" there.
Agree, the base
is confusing - and items in generators.base
are not functional as-is, iirc; no need to break that pattern.
Depending on the answer above, this seems like a discussion focused on needing a standard in naming expectations for generator classes. There is currently a mix of including the
model_type
value in the classname or not examples:
Agree. I disprefer repetition. I am missing a word for when the module's default generator is fine named by just the module, though, which is where we often see this.
Reifying:
- avoid repeating module name in generator class name
- strongly avoid repeating module name in non-default generator class name
- avoid including redundant information in class name
- have class name hint at modalities accepted (e.g. Chat, Vision, Completion)
Looking at the examples hinted:
These aren't exposed directly to users, but are overly verbose:
generators: litellm.LiteLLMGenerator
generators: octo 🌟 generators: octo.OctoGenerator
-- maybe these could become "Chat" or "Completion" or "Text"
These could do with a tidyup:
generators: nim 🌟 generators: nim.NVOpenAIChat generators: nim.NVOpenAICompletion
-- I'm not sure if the NV is redundant, but I expect third-party chat NIMs to continue using this API, and let's wait for a counterexample before working out a general solution. Beyond scope for this PR but could be e.g.
generators: nim.Chat generators: nim.Completion
These look pretty good to me, no repetition, and they generally describe the name of the interface using within the context:
generators: huggingface 🌟 generators: huggingface.ConversationalPipeline generators: huggingface.InferenceAPI generators: huggingface.InferenceEndpoint generators: huggingface.LLaVA generators: huggingface.Model generators: huggingface.OptimumPipeline generators: huggingface.Pipeline
generators: function 🌟 generators: function.Multiple generators: function.Single
generators: octo.InferenceEndpoint
generators: nim.Vision
-- one might rename huggingface.LLaVA
to huggingface.VisionLLaVA
for the modality constraint above, if LLaVa isn't considered clue enough.
I'd suggest leaving openai.OpenAIGenerator as-is.
@erickgalinkin can I ask what the vibes are like behind this leaning
@@ -190,7 +194,6 @@ def test_generator_structure(classname): | |||
"generators.huggingface.OptimumPipeline", # model name restrictions and cuda required | |||
"generators.huggingface.Pipeline", # model name restrictions | |||
"generators.langchain.LangChainLLMGenerator", # model name restrictions | |||
"generators.openai.OpenAICompatible", # template class not intended to ever be `Active` |
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.
nice catch
Signed-off-by: Jeffrey Martin <[email protected]>
fix #1008
Various generators extend this base template, usage and feedback suggest UX and usability improvements will be gained by promoting this class to be a
Configurable
generic OpenAI client based generator.Verification
List the steps needed to make sure this thing works
nim
configuration via generator config options:compatible.json:
groq
/openai.OpenAIGenerator
/nim
with--parallel_attempts
enabled.