Feature/multilingual #943
Feature/multilingual #943
Conversation
|
DCO Assistant Lite bot All contributors have signed the DCO ✍️ ✅ |
- Integrated support for multiple translation services including local and external APIs. - local: Huggingface model uses for translation - deepl:DeepL uses for translation - nim: NIM uses for translation - Implemented utility functions for language detection and text processing. Signed-off-by: Masaya Ogushi <mogushi@nvidia.com>
- Addd translation function for base probe class - prompts and triggers translate by base class method - attempt_descr translation Signed-off-by: Masaya Ogushi <mogushi@nvidia.com>
- Translation handling for detector keywords and substrings, triggers. Signed-off-by: Masaya Ogushi <mogushi@nvidia.com>
- Added support for specifying translation services directly from the CLI. - Implemented options to set target languages for translation. Signed-off-by: Masaya Ogushi <mogushi@nvidia.com>
- Added new dependencies required for enhanced translation features. Signed-off-by: Masaya Ogushi <mogushi@nvidia.com>
- Added detailed explanations of the translationn method - Included examples of how translation services are configured and utilized within the codebase. Signed-off-by: Masaya Ogushi <mogushi@nvidia.com>
SnowMasaya
force-pushed
the
feature/multilingual
branch
from
b781a6f to
6bb7da3
Compare
|
I have read the DCO Document and I hereby sign the DCO |
There was a problem hiding this comment.
Partial review, testing is still in progress.
The test failures in macOS look to be an incomplete dependency requirement that may need to be reworked or removed. A default installation should not require install of an external library. Hence the dependency on pyenchant here may be problematic.
Traceback:
/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/importlib/__init__.py:90: in import_module
return _bootstrap._gcd_import(name[level:], package, level)
tests/detectors/test_detectors_riskywords.py:8: in <module>
import garak.detectors.base
garak/detectors/__init__.py:1: in <module>
from .base import *
garak/detectors/base.py:17: in <module>
from garak.translator import SimpleTranslator, LocalTranslator, is_english
garak/translator.py:15: in <module>
import enchant
/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/site-packages/enchant/__init__.py:81: in <module>
from enchant import _enchant as _e
/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/site-packages/enchant/_enchant.py:[157](https://github.com/leondz/garak/actions/runs/11246708414/job/31296143918?pr=943#step:5:158): in <module>
raise ImportError(msg)
E ImportError: The 'enchant' C library was not found and maybe needs to be installed.
E See https://pyenchant.github.io/pyenchant/install.html
E for details
update remove punctuation update english judge add translate function add logging translate result add Reverse translate for hf detector and snowball probes Signed-off-by: Masaya Ogushi <mogushi@nvidia.com>
check translator instance remove translate function reset config Signed-off-by: Masaya Ogushi <mogushi@nvidia.com>
check translator instance add reverse translator add test reverse translator Signed-off-by: Masaya Ogushi <mogushi@nvidia.com>
remove argument using generator_option_file Signed-off-by: Masaya Ogushi <mogushi@nvidia.com>
add load translator instance Signed-off-by: Masaya Ogushi <mogushi@nvidia.com>
check storage size set up each instance for each test Signed-off-by: Masaya Ogushi <mogushi@nvidia.com>
remove pyenchant Using nltk instead of pyenchant Signed-off-by: Masaya Ogushi <mogushi@nvidia.com>
update how to use translation function Signed-off-by: Masaya Ogushi <mogushi@nvidia.com>
…garak into feature/multilingual
Signed-off-by: SnowGushiGit <snow.akogi.pgel@gmail.com>
…garak into feature/multilingual
There was a problem hiding this comment.
Thank you, this is a significant amount of work.
This is for translation-based multilingual, rather than general multilingual support, so we should be careful to separate these two functions. The ability to select which languages garak uses is distinct from how that language is achieved (in prompts, detectors, and so on).
There are some refactorings needed to get the PR to a sustainable state. We may need a couple more hooks, either injected into base classes or added there. It might be simplest to add these hooks to the harness. Happy to set up a call or instant message chat to discuss this. Given the breadth of the changes, it is likely to be beneficial to discuss plans and get good alignment while doing the rest of the changes.
There was a problem hiding this comment.
@SnowMasaya thank you, this is a significant benefit for the project.
Adding a number of my pending comments here, please be aware these are likely similar to many @leondz made, although possibly from different reasoning. Happy to continue iterating.
Indeed, sorry for the dupe review, there was quite a lot of code so I reviewed directly instead of processing @jmartin-tech 's comments as well |
add mean judge for reverse translation change translation model size Signed-off-by: Masaya Ogushi <mogushi@nvidia.com>
translate trigger words Signed-off-by: Masaya Ogushi <mogushi@nvidia.com>
add reverse translation remove trigger translation fix test code Signed-off-by: Masaya Ogushi <mogushi@nvidia.com>
Streamline translation use case
Signed-off-by: Jeffrey Martin <jemartin@nvidia.com>
* remote Riva do not serialize client object * remove extra call to get_generator in atkgen Signed-off-by: Jeffrey Martin <jemartin@nvidia.com>
Signed-off-by: Jeffrey Martin <jemartin@nvidia.com>
* probe() mints attempts and preforms reverse translation * output reverse translation is only called when prompts are translated * Attempt default bcp47 is "*" * Attempt only holds a single output reverse translation * harness no longer mutates attempt reverse outputs * detectors are not responible for tracking reverse translation * detectors select appropriate output or revese translated output Signed-off-by: Jeffrey Martin <jemartin@nvidia.com>
garak/translator.py
Outdated
| source_lang, target_lang = translation_service["language"].split("-") | ||
| if source_lang == target_lang: | ||
| return NullTranslator(translator_config) | ||
| model_type = translation_service["model_type"] |
There was a problem hiding this comment.
This code does not support remote case, I think we need to support for remote case.
For example,
if model_type == "remote":
model_type = f"{model_type}.{translation_service['class_name']}"
translator_instance = _plugins.load_plugin(
path=f"translators.{model_type}",
config_root=translator_config,
)
There was a problem hiding this comment.
model_type is treated the same as on the command line, and is documented to accept module or module.classname. If module only is provided per the plugin loading standard the loader will look for module.DEFAULT_CLASS for a class name to instantiate.
Given this config:
run:
lang_spec: ja
translator:
- language: en-ja
model_type: remote
- language: ja-en
model_type: remote.RivaTranslator
The en-ja translator would lookup the garak.translators.remote.DEFAULT_CLASS which is RivaTranslator and the ja-en translator would load the same translator a the more specific class is specified.
To use the the Deepl translator the more specific class must be specified in model_type:
run:
lang_spec: ja
translator:
- language: en-ja
model_type: remote.DeeplTranslator
api_key: ""
- language: ja-en
model_type: remote.DeeplTranslator
api_key: ""
There was a problem hiding this comment.
Your comment here identified a bug in the Configurable tooling, the module.classname attributes are not being applied correctly in the that class. I will open a separate PR to ensure that functionality is working as expected.
While remote.RivaTranslator and remote.DeeplTranslator do work at this time if the environment variables are exported only default configuration is working. Custom configuration such as uri override or api_key is only working if just the module name remote is provided as model_type.
* params for remote translation are basic string not list * modify test example translation configs to match supported format Signed-off-by: Jeffrey Martin <jemartin@nvidia.com>
* improve plugin load error handling * load translation on harness initialization Signed-off-by: Jeffrey Martin <jemartin@nvidia.com>
* set multiprocessing options when loading HF model for consistency * remove excessive logging from translator as captured in attempts * note possible future edge case handling needs Signed-off-by: Jeffrey Martin <jemartin@nvidia.com>
* remove remote attributes from base `Translator` * always set self.client when `_load_translator()` is called Signed-off-by: Jeffrey Martin <jemartin@nvidia.com>
* revert atkgen probe from `main` * notes[turns] contains original prompt and reverse translated response * Attempt.messages now contain actual values sent and recieved from the target * adjust atkgen probe translation expectations Signed-off-by: Jeffrey Martin <jemartin@nvidia.com>
|
rigochat not working: |
|
would be good to know what the download /is/ (guessing translator) - this isn't reported in the CLI, but the progress bars are. would also be good to know what translation is requested, when requested (maybe bulleted with 🌐). |
|
There's a long delay after a probe progress bar completes for the first time. What's happening? My guess is that a translation model is loading. We should inform CLI user |
This better reflects the module responsibilty as enabling language specific support as a service to other components. Signed-off-by: Jeffrey Martin <jemartin@nvidia.com>
Signed-off-by: Jeffrey Martin <jemartin@nvidia.com>
This better reflects the expectation that a single language is expected to be targeted during the run. Signed-off-by: Jeffrey Martin <jemartin@nvidia.com>
Signed-off-by: Jeffrey Martin <jemartin@nvidia.com>
Signed-off-by: Jeffrey Martin <jemartin@nvidia.com>
* indicate load of translation services in harness init * removed internal additional base.Harness from extending classes Signed-off-by: Jeffrey Martin <jemartin@nvidia.com>
There was a problem hiding this comment.
Great lift, thanks. Some renaming and refactoring and explanation requests. Themes include:
- source & target lang need to explicit more places
- need to prune down default imports
- some variable/method renames to be precise (e.g. only using translator for things doing translation)
- need probes to be able to choose to not-translate
| msg = ( | ||
| f"A possibly secret value (`api_key`) was detected in {settings_filename}. " | ||
| f"We recommend removing potentially sensitive values from config files or " | ||
| f"ensuring the file is readable only by you." | ||
| ) |
There was a problem hiding this comment.
What is this black churn that we have going on? There was once a pre-commit hook for running black but I'm starting to think it never left my local machine - thinking that would be a fine place for documenting the code formatting expectation
| @@ -391,7 +396,7 @@ def load_plugin(path, break_on_fail=True, config_root=_config) -> object: | |||
| except ValueError as ve: | |||
| if break_on_fail: | |||
| raise ValueError( | |||
| f'Expected plugin name in format category.module_name.class_name, got "{path}"' | |||
| f'Expected plugin name in format category.module_name or category.module_name.class_name, got "{path}"' | |||
There was a problem hiding this comment.
can see a confusion here (cat.mod only actually works for generator), have put more common case as first example
| f'Expected plugin name in format category.module_name or category.module_name.class_name, got "{path}"' | |
| f'Expected plugin name in format category.module_name.class_name or category.module_name, got "{path}"' |
| @@ -38,6 +38,10 @@ class Attempt: | |||
| :type seq: int | |||
| :param messages: conversation turn histories; list of list of dicts have the format {"role": role, "content": text}, with actor being something like "system", "user", "assistant" | |||
| :type messages: List(dict) | |||
| :param bcp47: Language code for prompt as sent to the target | |||
There was a problem hiding this comment.
so.. dest_bcp47? the attempt isn't guaranteed to have one single language, so having one single / default bcp47 fosters confusion
| @@ -72,6 +76,8 @@ def __init__( | |||
| detector_results=None, | |||
| goal=None, | |||
| seq=-1, | |||
| bcp47="*", # language code for prompt as sent to the target | |||
There was a problem hiding this comment.
* denotes language-agnostic - this is a perhaps over-opinionated default and requires that probes explicitly alter the value for any attempts involving natural language
| @@ -62,13 +62,20 @@ def __init__(self, config_root=_config): | |||
| ) | |||
|
|
|||
| logging.info(f"detector init: {self}") | |||
| self.reverse_translator = self.get_reverse_translator() | |||
There was a problem hiding this comment.
This should be gated on an established need for a reverse translator. If we're not doing translation, we shouldn't get calling out. There are no guarantees that get_reverse_translator() will remain a lightweight method, so it's not a great thing to include unconditionally in a base class def.
| mean_word_judge = is_meaning_string(prompt) | ||
| if mean_word_judge: | ||
| translate_prompt = self._get_response(prompt) | ||
| translated_prompts.append(translate_prompt) | ||
| else: | ||
| translated_prompts.append(prompt) |
| translated_attempt_descrs.append( | ||
| str( | ||
| { | ||
| "prompt_stub": translate_prompt_stub, | ||
| "distractor": descr["distractor"], | ||
| "payload": translate_payload, | ||
| "az_only": descr["az_only"], | ||
| "use refocusing statement": descr["use refocusing statement"], | ||
| } | ||
| ) |
There was a problem hiding this comment.
This looks closely fit to maybe goodside.Tag? What is it?
| ("authorization", "Bearer " + self.api_key), | ||
| ], | ||
| ) | ||
| self.client = riva.client.NeuralMachineTranslationClient(auth) |
There was a problem hiding this comment.
I guess if this raises an exception we don't need to catch it
| target_lang = "EN-US" if self.target_lang == "en" else self.target_lang | ||
| return self.client.translate_text( | ||
| text, source_lang=self.source_lang, target_lang=target_lang | ||
| ).text |
There was a problem hiding this comment.
we may need to handle casing - garak is bcp47 compliant but DeepL looks like it expects uppercase two-character codes
| @@ -75,6 +75,34 @@ def __init__(self, config_root=_config): | |||
| self.description = self.__doc__.split("\n", maxsplit=1)[0] | |||
| else: | |||
| self.description = "" | |||
| self.translator = self.get_translator() | |||
There was a problem hiding this comment.
Probes need to be able to opt-out of translation (see e.g. issue #1066 )
* all base Probes store original prompt * atkgen probe store original prompt * misleading detector premise uses original prompt Signed-off-by: Jeffrey Martin <jemartin@nvidia.com>
jmartin-tech
force-pushed
the
feature/multilingual
branch
from
1a300d6 to
8f9fa1b
Compare
This revision enables translation support for a streamlined the use case. Some items such
as support for multiple target languages in a single run have been omitted in favor of a
more straight forward user experience and reduced ambiguity in the scope of results.
Translator classes have been implemented using the
Configurablepattern and the pluginloader. This introduced a new paradigm of providing configuration for a list of instances
with specific configuration required at runtime where previous
Configurableclassconfiguration has been for all instances of a specific class or module. The processing and
attribute names used to create this instance list may evolve further.
Usage
Translation function is configured in the
runsection of a configuration see the docpage in the PR for details.
New default configuration values for
run.target_langandrun.translatorsare in theupdated documentation and allow for backwards compatible configuration with existing runs.
There are still some existing
TODO:comments and notes about location that may needfurther testing before landing this upstream. Most noteworthy are comments still in the
code of the
atkgenprobe that require further scrutiny to validate the attack techniqueis applied correctly.
It may be appropriate to gate this functionality as experimental for initial release, this
would required some additional guard code to ensure limited impact to report formats and
internal state.
Example
hf_RigoChat_gpu.yml: