Add comet scorer #118
Add comet scorer #118
Conversation
|
Thanks for your PR! I think it would be better to use a single block of code for all comet-like scorer and add a setting for the model path at start-up. |
|
In a real world, COMET requires a GPU so it would be great to unload the model being trained or/and specify another gpu for comet. What do you think ? |
eole/scorers/comet.py
Outdated
| if self.model_name == "COMET-KIWI": | ||
| for _src, _hyp in zip(texts_srcs, preds): | ||
| current_segment = {"src": _src, "mt": _hyp} | ||
| data.append(current_segment) | ||
| else: | ||
| for _src, _tgt, _hyp in zip(texts_srcs, texts_refs, preds): | ||
| current_segment = {"src": _src, "mt": _hyp, "ref": _tgt} | ||
| data.append(current_segment) |
There was a problem hiding this comment.
Not super fan of this condition, but I guess that works. Not sure we could do much cleaner without defining subclasses for each metric, which would probably be overkill here.
eole/scorers/comet.py
Outdated
| data.append(current_segment) | ||
| if len(preds) > 0: | ||
| score = self.comet_model.predict( | ||
| data, batch_size=64, gpus=0, num_workers=0, progress_bar=True |
There was a problem hiding this comment.
batch_size is a bit hidden here. Do we expect to need to change this at some point?
Might not be necessary to have a dedicated flag for this, but maybe set this explicitly in the init so that it's a bit more explicit.
There was a problem hiding this comment.
Maybe we should let these 3 (4?) option to be customized indeed
|
Continuing the discussion here: |
|
I think the easiest for 1) user convenience 2) speed is to unload the model being trained, load the comet model on the same gpu, and relaod the trianed model after scoring. Scoring on cpu will be way too slow and dedicating a second gpu will be a nightmare to handle from the user standpoint |
|
Tested how vram was impacted with this method: This leaves a 300mib trace in vram. |
Added comet scorers for validation in NMT training.
I left
progress_bar=True,num_workers=0,batch_size=64andgpu=0for cpu inference, should it be like that by default and should it be modifiable via config ?I did not modify tests as it would greatly extend the tests duration (downloading comet models and inferencing on cpu).
I also took the liberty to lightly modify wmt17 recipe/doc to make it more beginner friendly.
Fix #34