Skip to content
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

Configurable Generator parameters #589

Closed
erickgalinkin opened this issue Apr 9, 2024 · 13 comments · Fixed by #711 or #742
Closed

Configurable Generator parameters #589

erickgalinkin opened this issue Apr 9, 2024 · 13 comments · Fixed by #711 or #742
Assignees
Labels
architecture Architectural upgrades bug Something isn't working generators Interfaces with LLMs
Milestone

Comments

@erickgalinkin
Copy link
Collaborator

We need a way to instantiate generators in a way that:

  • Accepts a device_map for multi-gpu systems
  • Accepts a dtype (so we can load torch.bfloat16, for example)
@erickgalinkin erickgalinkin added bug Something isn't working architecture Architectural upgrades generators Interfaces with LLMs labels Apr 9, 2024
@erickgalinkin erickgalinkin self-assigned this Apr 9, 2024
@jmartin-tech
Copy link
Collaborator

As a note this kind of customization of the generator feels like something that should be standardized as config options that can be passed via the -G options similar to how rest options are passed.

@jmartin-tech
Copy link
Collaborator

Link #554

@leondz leondz self-assigned this Apr 10, 2024
@leondz
Copy link
Collaborator

leondz commented Apr 10, 2024

trying to get this running via the standard config. the current configs don't auto-populate plugin class values, only at module level, which makes this a pain. will get that working better in the plugin loader

@leondz
Copy link
Collaborator

leondz commented Apr 10, 2024

Oh cool (narrator voice: it was not, in fact, cool): generators aren't loaded with _plugins.load_plugin, or any of the usual plugin loading machinery, so their config never gets automatically set up. I did wonder how the plugin loader would handle generator instantiation params (just model_name).

Actions:

  • move generator loading to use plugin loading & configuation architecture - find a way to pass model_name at instantiation time (maybe a special case in load_plugin eh) - tracked in standardise generator loading #594
  • update the configuration machinery to take class-level info as it looks in _plugins / _config. this is nested dataclasses then nested dicts - it's not completely palatable but is functional. prefer keeping the yaml format looking sensible - tracked in update the configuration machinery to take class-level info #595
  • will set up a hugging face mixin that loads huggingface module-level config into the HF classes, so we can carry on working while these two issues are addressed

@leondz
Copy link
Collaborator

leondz commented Apr 11, 2024

When we work out device_map - and this feels global rather than huggingface related, is that right? - we should

  • look for mentions of cuda / maybe just cuda: in the code and test that they're not overriding it
  • write test requesting cpu, cuda default device, and cuda specific non-first device (skipping if unavailable) and verifying that they're used

@leondz leondz added this to the rel milestone Apr 18, 2024
@leondz
Copy link
Collaborator

leondz commented Apr 22, 2024

seems like device_map is a hf-dependent concept, so _config.plugins.generators.huggingface makes more sense as a home

@jmartin-tech
Copy link
Collaborator

I think from a generic point of view, we could have a the Configurable functionality in generators/base.py as a dict and provide validation of parameters in the class before consuming the values. Each generator can then determine how to handler the dict, in some cases by providing a set of expected valid keys or by just validating the keys and values are all in a format that will not present easy entry point for RCE based on generators configuration pattern.

@leondz leondz modified the milestones: rel, release 0.9.1 Apr 23, 2024
@leondz leondz changed the title Huggingface Generator parameters Configurable Generator parameters May 2, 2024
@leondz leondz changed the title Configurable Generator parameters Configurable Generator parameters May 2, 2024
@leondz
Copy link
Collaborator

leondz commented May 2, 2024

folding #594 into here, check out notes there

@leondz
Copy link
Collaborator

leondz commented May 2, 2024

folding #643 into here, check out notes there

@leondz
Copy link
Collaborator

leondz commented May 2, 2024

There's an example of generators loading centralised configs in RestGenerator; see https://github.com/leondz/garak/blob/075796f044c2877977383dcf561b5a54f865c009/garak/generators/rest.py#L113...L149

@kbmlcoding
Copy link

Does this means we wont be able to run garak for any model that does not fit one single gpu until multi-gpu feature from this pull request is merged to main and release? @leondz

@leondz
Copy link
Collaborator

leondz commented Jun 5, 2024

You can hardcode for now, but #711 is expected to land this week which paves the way to exposing this.

@kbmlcoding
Copy link

kbmlcoding commented Jun 5, 2024

Awesome .. that is great @leondz ..where can i find the instructions to build garak locally on either mac/linux system if i were to make hardcoded change you suggested

Assuming all i need to do is :
change https://github.com/leondz/garak/blob/7839ee33c5b2f6568204205086aff183653cc4f7/garak/generators/huggingface.py#L476
to
self.init_device = "auto"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment