fix: load detoxify model from state dict and upgrade transformers version #180
Conversation
| TODO: To be switched to consuming HF model once consistency issue is resolved: | ||
| https://huggingface.co/unitary/unbiased-toxic-roberta. This will allow removing detoxify PyPI as a dependency, | ||
| update transformers version we are consuming. | ||
| """ | ||
|
|
||
| DETOXIFY_MODEL_TYPE = "unbiased" | ||
| UNBIASED_MODEL_URL = ( | ||
| "https://github.com/unitaryai/detoxify/releases/download/v0.3-alpha/toxic_debiased-c7548aa0.ckpt" |
There was a problem hiding this comment.
Should we add this file directly to the fmeval repo so that we don't rely on the detoxify repo? This isn't a major concern (non-blocking).
There was a problem hiding this comment.
Let's discuss this with science team first, if we need to check on it with legal.
| state_dict=state_dict["state_dict"], | ||
| local_files_only=False, | ||
| ) | ||
| .to("cpu") |
There was a problem hiding this comment.
I think this is fine for now, but we should ideally identify whether any GPUs exist, and if so, place the model on the GPU instead.
There was a problem hiding this comment.
+1
also if map_location is used I don't think you need a to('cpu') at the end
| TODO: To be switched to consuming HF model once consistency issue is resolved: | ||
| https://huggingface.co/unitary/unbiased-toxic-roberta. This will allow removing detoxify PyPI as a dependency, | ||
| update transformers version we are consuming. | ||
| """ | ||
|
|
||
| DETOXIFY_MODEL_TYPE = "unbiased" | ||
| UNBIASED_MODEL_URL = ( | ||
| "https://github.com/unitaryai/detoxify/releases/download/v0.3-alpha/toxic_debiased-c7548aa0.ckpt" |
There was a problem hiding this comment.
Let's discuss this with science team first, if we need to check on it with legal.
| state_dict=state_dict["state_dict"], | ||
| local_files_only=False, | ||
| ) | ||
| .to("cpu") |
There was a problem hiding this comment.
+1
also if map_location is used I don't think you need a to('cpu') at the end
| @@ -123,7 +140,16 @@ def get_helper_scores(self, text_input: List[str]) -> Dict[str, List[float]]: # | |||
| :param text_input: list of text inputs for the model | |||
| :returns: dict with keys as score name and value being list of scores for text inputs | |||
| """ | |||
| return self._model(text_input) | |||
| inputs = self._tokenizer(text_input, return_tensors="pt", truncation=True, padding=True).to(self._model.device) | |||
| scores = torch.sigmoid(self._model(**inputs)[0]).cpu().detach().numpy() | |||
There was a problem hiding this comment.
It's unclear if this is supposed to work in a batch call or not. why do you select self._model(**inputs)[0]? Are we assuming text_input is a list with only one string?
There was a problem hiding this comment.
self._model(**inputs) returns an object of class SequenceClassifierOutput, where the [0] is the location of the tensor containing model output values. The tensor can be two dimensional so batching is still supported here, and our helper model unit test with multiple string inputs also passed.
This was referenced from detoxify repo's predict method.
There was a problem hiding this comment.
Ok thanks! Please add some inline comments for future reference :)
| for i, cla in enumerate(DetoxifyHelperModel.get_score_names()): | ||
| results[cla] = ( | ||
| scores[0][i] | ||
| if isinstance(text_input, str) |
There was a problem hiding this comment.
indeed, from signature text_input should be a List. I don't think this method works if text_input is a list with more than one string (because of line 144)
| @@ -42,7 +42,7 @@ def test_evaluate_sample(self, integration_tests_dir): | |||
| elif eval_score.name == ROUGE_SCORE: | |||
| assert eval_score.value == approx(0.250, abs=ABS_TOL) | |||
| elif eval_score.name == BERT_SCORE: | |||
| assert eval_score.value == approx(0.734, abs=ABS_TOL) | |||
| assert eval_score.value == approx(0.748, abs=ABS_TOL) | |||
There was a problem hiding this comment.
Upgrading transformers version to ^4.36.0 caused the bertscore values to be slightly different.
I did some testing and found that bertscore output differs for < v4.24.0 and >= v4.24.0, (previously we were using v4.22.1). This issue is similar to what was observed in this github issue.
I wasn't able to find the root cause for the change but it seems like this occurs sometimes from pytorch/transformers upgrades, see previous issue.
Issue #, if available:
https://tiny.amazon.com/f6f228ty/issuamazissuRAI7
We need to update transformers to >=
v4.36.0due to security vulnerabilities, butthe latest detoxify
v0.5.1requires transformersv4.22.1. Additionally, the current method for loading models in detoxify errors with transformers >v4.30.0, see issue.Description of changes:
To resolve the dependency conflict and model loading issue, this PR:
load_checkpointwith modifications to address the issue above.^4.36.0to resolve security vulnerabilities. (This caused bertscore metric to output slightly different values, where bertscore output differs for <v4.24.0and >=v4.24.0, similar to in this issue).By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.