[Marian] documentation and AutoModel support #4152
Conversation
sshleifer
changed the title
tests/test_modeling_marian.py
Outdated
| # bad_words_ids=[[self.tokenizer.pad_token_id]], | ||
| decoder_start_token_id=self.tokenizer.pad_token_id, # mimics 0 embedding at first step. | ||
| ) | ||
| generated_words = self.tokenizer.decode_batch(generated_ids, skip_special_tokens=True) |
There was a problem hiding this comment.
Just noticed that there is a decode_batch function in the Marian tokenizer. I think we should remove this or is it planned that this function is also in tokenizer_utils.py ? I think we should keep each models API in general as small as possible and try to only expose the common _utils functions.
IMO, the user will get used ot the function decode_batch and wonder why it does not exist for other tokenizers and not for the FastTokenizer either. Or we should implement a general decode_batch function in tokenization_utils , but not for an individual tokenizer.
There was a problem hiding this comment.
There was a problem hiding this comment.
Would love to hoist it up to PretrainedTokenizer.
#4159 does that.
There was a problem hiding this comment.
Fine for me.
Maybe let's use the same naming convention with batch_decode indeed (not that I'm a big fan of the one we have right now for batch_encode but it's better to be consistent indeed)
| "Tom really admired Mary's courage.", | ||
| "Turn around and close your eyes.", | ||
| ] | ||
| expected_text = [ |
There was a problem hiding this comment.
Great translations :-)
| def parse_readmes(repo_path): | ||
| def make_registry(repo_path="Opus-MT-train/models"): | ||
| if not Path(repo_path).exists(): | ||
| raise ValueError("You must run: git clone git@github.com:Helsinki-NLP/Opus-MT-train.git") |
There was a problem hiding this comment.
Maybe specify that the repo_path was invalid?
Co-authored-by: Lysandre Debut <lysandre@huggingface.co>
tests/test_modeling_marian.py
Outdated
| # bad_words_ids=[[self.tokenizer.pad_token_id]], | ||
| decoder_start_token_id=self.tokenizer.pad_token_id, # mimics 0 embedding at first step. | ||
| ) | ||
| generated_words = self.tokenizer.decode_batch(generated_ids, skip_special_tokens=True) |
There was a problem hiding this comment.
Fine for me.
Maybe let's use the same naming convention with batch_decode indeed (not that I'm a big fan of the one we have right now for batch_encode but it's better to be consistent indeed)
sshleifer
changed the title
Metrics:
For fr-en test set:
MarianMTModel.from_pretrained('Helsinki-NLP/opus-mt-fr-en'): 57.4817