Add ability to switch output languages for multilingual models #69 #72
Conversation
This reverts commit 075c4d3.
|
My questions are:
|
facebook-github-bot
added
the
CLA Signed
|
You are on the right track. To keep it simple, for now consider this as the use case for which you are making the changes: When running the example pipeline context of target_language.txt: Now we want dataloader to load this file along with the source and target references, and then use the target language value from that file for setting the target language in the policy method of |
Given the above use case, it would be required to make some necessary changes in dataloader.
Even in the use case I stated, we are not technically changing the target language dynamically. But the changes you make in SimulEval to make things work for this use case will pave the way to dynamically pass in the target language from the demo front end. In the given use case, instead of passing the target language as a parameter when loading the pipeline (i.e., |
simuleval/data/segments.py
Outdated
| @@ -15,6 +15,7 @@ class Segment: | |||
| finished: bool = False | |||
| is_empty: bool = False | |||
| data_type: str = None | |||
| tgt_lang: str = "" | |||
There was a problem hiding this comment.
I can see that you have made some changes to add target language as a property to dataloader and segment classes. But I don't see any change for passing the target language parameter from the dataloader to the segment.
In case you are wondering, the instance class (eg: https://github.com/facebookresearch/SimulEval/blob/main/simuleval/evaluator/instance.py) is the one that loads a specific instance/sample from the dataloader and then creates segments for that instance.
There was a problem hiding this comment.
Thanks so much for the comments, they are super helpful!
|
I made new changes and updated the instance.py to pass the tgt_lang to the segment when creating it. It seems to work well only that the target language is not detected and instances.log shows "unknown" where the target language is meant to be when I used the dummy model (counter_in_tgt_lang_agent.py). I suspect that the target language parameter (args) from the counter_in_tgt_lang_agent is not getting passed to the instance. |
|
Are you sure all changes have been pushed to this PR? |
|
This is the summary of changes I think that needs to be made:
|
I am sure I have touched on all these aspects. |
|
@SamDewriter If you are looking for more code pointers, this should help: The final missing part will be the passing of the tgt_lang from segment to the agent's policy method. This is the main part that is going to be useful when making changes to the demo.
|
|
In this new commit, I pass the target language to only the SpeechInputInstance class. The property tgt_lang is not added to the parent class instance. It was commented out in the last commit. |
| if args is not None: | ||
| with open(args.tgt_lang, "r") as file: | ||
| tgt_lang = file.read() | ||
| self.tgt_lang = tgt_lang |
There was a problem hiding this comment.
Is this part here as a fallback? It is fine to leave it in as a fallback, but if this is the sole logic for getting tgt_lang in this agent then it won't meet our purpose.
We primarily want to get the tgt lang param dynamically passed to the policy method instead of solely getting set at initialization. You understand that solely getting set at initialization means it will remain static, right?
There was a problem hiding this comment.
The reason I added this part is just to preprocess the tgt_lang because we are reading it from a file. I came to this conclusion after several trials and errors. I noticed that before adding the logic, the evaluation works well but the tgt_lang is not being passed.
There was a problem hiding this comment.
One thing I'd also need clarification on is that in the counter_in_tgt_lang_agent, the tgt_lang is being passed to the class after inheriting from the parent class Agent, which means that the tgt_lang is not implemented in the parent class. I suppose the right thing will be to implement it in the parent class so that all the children classes will automatically have it
There was a problem hiding this comment.
We don't want to set tgt_lang in the init method by the end of this effort, so don't worry about moving this attribute to the parent class for now.
If the objective is still not clear to you, imagine that
SimulEval/examples/speech_to_text/counter_in_tgt_lang_agent.py
Lines 22 to 24 in c7a1749
policy method as part of the states, something like:
def policy(self, states: Optional[AgentStates] = None):
....
....
tgt_lang = states.tgt_lang
if tgt_lang == "en":
prediction += "seconds"
elif tgt_lang == "es":
prediction += "segundos"
elif tgt_lang == "de":
prediction += "sekunden"
else:
prediction += "<unknown>"
....
....
There was a problem hiding this comment.
Oh! Now I understand the goal. Thanks for the clarification!
|
The changes in latest commit looks good. |
Here contains the few changes I have made, which I am doubting makes sense.