Add ability to switch output languages for multilingual models #69 #74

SamDewriter · 2023-08-28T16:49:10Z

Changes have been made to add the ability to switch output languages for multilingual models.

This reverts commit 075c4d3.

ibanesh

Thanks @SamDewriter for making the requested changes.
This iteration seems to be on the right track, but we still needs some modification before it is ready to be merged.

ibanesh · 2023-09-07T18:37:22Z

simuleval/options.py

+    # parser.add_argument(
+    #     "--tgt-lang",
+    #     type=str,
+    #     default=None,
+    #     help="Path to the Target language file.",
+    # )


Suggested change

# parser.add_argument(

# "--tgt-lang",

# type=str,

# default=None,

# help="Path to the Target language file.",

# )

Please clean up this remnant commented code.

ibanesh · 2023-09-07T20:22:26Z

simuleval/evaluator/instance.py

+        if self.dataloader is not None:
+            self.tgt_lang = self.dataloader.tgt_lang
+            if isinstance(self.tgt_lang, list):
+                if index < len(self.tgt_lang):
+                    self.tgt_lang = self.tgt_lang[self.index]
+                else:
+                    self.tgt_lang = None
+        else:
+            self.tgt_lang = None


This might work, but is not the cleanest way to implement the tgt_lang assignment for instances.

Like I had pointed out earlier, we want something similar to how the source and target getters work.
https://github.com/facebookresearch/SimulEval/blob/main/simuleval/evaluator/instance.py#L46-L48
https://github.com/facebookresearch/SimulEval/blob/main/simuleval/data/dataloader/dataloader.py#L55-L56

In case it is not clear, we want to be able to do something like self.tgt_lang = self.dataloader[index]["tgt_lang"] to access the target language of a particular index.

Another cause for concern here is that all dataloaders are going to have the tgt_lang attribute.
There are some dataloaders (in internal fairseq repo), which don't have the tgt_lang attribute that you added in SpeechToTextDataloader or SpeechToSpeechDataloaderin this PR and self.dataloader.tgt_lang would throw an exception if the dataloader doesn't have this attribute defined correctly. So, please keep in mind while making these changes that not all dataloader have the tgt_lang attribute and handle it accordingly.

Also, at this point feel free to factor out and move necessary changes to the base class GenericDataloader whenever it makes sense to do so.

ibanesh

Looks mostly good! We are almost there, just need a few minor changes and cleanup.

ibanesh · 2023-09-13T23:55:20Z

simuleval/agents/agent.py

@@ -184,6 +184,7 @@ class SpeechToTextAgent(GenericAgent):

    source_type: str = "speech"
    target_type: str = "text"
+    tgt_lang: str = None


Suggested change

tgt_lang: str = None

tgt_lang: Optional[str] = None

ibanesh · 2023-09-13T23:58:28Z

simuleval/agents/states.py

        elif isinstance(segment, SpeechSegment):
            self.source += segment.content
            self.source_sample_rate = segment.sample_rate
+            self.tgt_lang = segment.tgt_lang


I don't see any reason to restrict this just to the SpeechSegment case, can you add the tgt_lang part to TextSegment case too.

ibanesh · 2023-09-14T00:13:38Z

simuleval/data/dataloader/dataloader.py

@@ -52,15 +56,28 @@ def get_source(self, index: int) -> Any:
    def get_target(self, index: int) -> Any:
        return self.preprocess_target(self.target_list[index])

+    def get_tgt_lang(self, index: int) -> Any:
+        if self.tgt_lang_list is not None and index < len(self.tgt_lang_list):
+            return self.preprocess_tgt_lang(self.tgt_lang_list[index])


Don't thin we need a preprocessor for tgt_lang attribute, preprocessor makes more sense for source and targets which can potentially be sound files.

ibanesh · 2023-09-14T00:14:13Z

simuleval/data/dataloader/dataloader.py

+    def preprocess_tgt_lang(self, tgt_lang: Any) -> Any:
+        raise NotImplementedError
+


Suggested change

def preprocess_tgt_lang(self, tgt_lang: Any) -> Any:

raise NotImplementedError

Also, even if we want a a preprocessor for tgt_lang, throwing a NotImplementedError here would break some of the dataloaders (not visible to you) that derives from this base class and don't have this new method implemented.

ibanesh · 2023-09-14T00:19:56Z

simuleval/data/dataloader/dataloader.py

@@ -52,15 +56,28 @@ def get_source(self, index: int) -> Any:
    def get_target(self, index: int) -> Any:
        return self.preprocess_target(self.target_list[index])

+    def get_tgt_lang(self, index: int) -> Any:


Suggested change

def get_tgt_lang(self, index: int) -> Any:

def get_tgt_lang(self, index: int) -> Optional[str]:

ibanesh · 2023-09-14T00:28:18Z

simuleval/data/segments.py

@@ -34,12 +35,14 @@ class EmptySegment(Segment):
 class TextSegment(Segment):
    content: str = ""
    data_type: str = "text"
+    tgt_lang: str = ""


Suggested change

tgt_lang: str = ""

tgt_lang: Optional[str] = None

ibanesh · 2023-09-14T00:28:31Z

simuleval/data/segments.py



 @dataclass
 class SpeechSegment(Segment):
    sample_rate: int = -1
    data_type: str = "speech"
+    tgt_lang: str = ""


Suggested change

tgt_lang: str = ""

tgt_lang: Optional[str] = None

ibanesh · 2023-09-14T00:29:39Z

simuleval/data/dataloader/s2t_dataloader.py

-        dataloader = cls(source_list, target_list)
+        with open(tgt_lang) as f:
+            tgt_lang_list = [line.strip() for line in f]
+            print(tgt_lang_list)


cleanup

Suggested change

print(tgt_lang_list)

ibanesh · 2023-09-14T00:33:03Z

simuleval/evaluator/instance.py

@@ -46,6 +46,13 @@ def __init__(
        if self.dataloader is not None:
            self.source = self.dataloader[self.index]["source"]
            self.reference = self.dataloader[self.index]["target"]
+
+            # Handle when tgt_lang is not provided


Suggested change

# Handle when tgt_lang is not provided

The comment here seems redundant, it is obvious from the code.

ibanesh · 2023-09-14T00:35:46Z

simuleval/evaluator/instance.py

+            if self.dataloader.tgt_lang_list is not None:
+                self.tgt_lang = self.dataloader[self.index]["tgt_lang"]
+            else:
+                self.tgt_lang = None


Suggested change

if self.dataloader.tgt_lang_list is not None:

self.tgt_lang = self.dataloader[self.index]["tgt_lang"]

else:

self.tgt_lang = None

if self.dataloader.tgt_lang_list is not None and self.index < len(self.dataloader.tgt_lang_list):

self.tgt_lang = self.dataloader[self.index]["tgt_lang"]

else:

self.tgt_lang = None

…ulEval into dynamic_language

ibanesh · 2023-09-20T06:29:42Z

simuleval/evaluator/instance.py

+
+            if self.dataloader.tgt_lang_list is not None and self.index < len(
+                self.dataloader.tgt_lang_list
+            ):
+                self.tgt_lang = self.dataloader[self.index]["tgt_lang"]
+            else:
+                self.tgt_lang = None


On second thought, this check here is redundant as we already do this check in the dataloader

Suggested change

if self.dataloader.tgt_lang_list is not None and self.index < len(

self.dataloader.tgt_lang_list

):

self.tgt_lang = self.dataloader[self.index]["tgt_lang"]

else:

self.tgt_lang = None

self.tgt_lang = self.dataloader[self.index]["tgt_lang"]

…dynamic_language

ibanesh

@SamDewriter Thanks for making the requested changes. This looks good and almost ready to be merged.

ibanesh · 2023-09-20T17:55:58Z

simuleval/data/dataloader/s2t_dataloader.py

+        tgt_lang_list = []
+        if tgt_lang is not None:
+            with open(tgt_lang) as f:
+                tgt_lang_list = [line.strip() for line in f]
+        dataloader = cls(source_list, target_list, tgt_lang_list)


Great work on catching that S2S dataloader is missing this part and adding it!

ibanesh · 2023-09-20T17:59:00Z

simuleval/data/dataloader/s2t_dataloader.py

+        with open(tgt_lang) as f:
+            tgt_lang_list = [line.strip() for line in f]


Can you add the null check for tgt_lang similar to what you added in the S2S dataloader. Good job on adding the null check in the S2S dataloader.

ibanesh

Looks good to me!

Mubaraq Sani added 28 commits July 19, 2023 21:28

Testing Circleci on main

89fbc24

Merge branch 'main' of https://github.com/SamDewriter/SimulEval

93f9643

Testing Circleci on main

fd41dc2

Testing Circleci on main

4becf02

Testing Circleci on main

cc893ef

Testing Circleci on main

a6f00f8

correct Circle config

0f41351

correct Circle config

78d13d0

correct Circle config

98ae6ec

correct Circle config

cdffc8d

Revert "[demo] s2t + s2s agent pipelines (facebookresearch#58)"

cacfbc9

This reverts commit 075c4d3.

resolve branch changes

f97cdfa

add target language

4ddf84a

add target language as a parameter

69e5816

Test dynamic language

812bcbc

Switch language dynamically

233aa35

Add ability to switch output language (facebookresearch#69)

f15634c

Add tgt language argument

c06fb0c

Add Namespace to args argument (facebookresearch#69)

5e0165a

Modify code to read target language from a file

c43e9da

Add ability to switch input language

bbe6a88

Add a tgt-lang file to test

03c06b1

Add tgt_lang to instance

9d97d18

Add tgt_lang to AgentStates

3e5fbe6

States

6d06a8e

Add tgt_lang from state to test (facebookresearch#69)

50efb42

Target language to test

1c07b35

Format with Black (facebookresearch#69)

0627451

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 28, 2023

Delete circleci (69)

3a63e16

Mubaraq Sani added 3 commits September 6, 2023 19:19

correct Circle config

2ea30a9

correct Circle config

4377457

Recover deleted files

0521818

ibanesh reviewed Sep 7, 2023

View reviewed changes

Mubaraq Sani added 5 commits September 13, 2023 17:04

Move tgt-lang to DataLoader (facebookresearch#69)

fa3b94c

Rewrite tgt_lang (facebookresearch#69)

5d72f50

Initialize tgt-lang (facebookresearch#69)

a6d3a58

Format with black

e8b4517

Fix tgt-lang (facebookresearch#69)

609d282

ibanesh reviewed Sep 14, 2023

View reviewed changes

Correct tgt_lang logic

f6b8cde

SamDewriter requested a review from ibanesh September 17, 2023 11:46

SamDewriter and others added 3 commits September 17, 2023 12:51

Merge branch 'main' into dynamic_language

cca97aa

Resolve merge conflict

54b1fb6

Merge branch 'dynamic_language' of https://github.com/SamDewriter/Sim…

6b9e465

…ulEval into dynamic_language

ibanesh reviewed Sep 20, 2023

View reviewed changes

Mubaraq Sani added 6 commits September 20, 2023 17:12

remove tgt_lang check to reduce redundancy (facebookresearch#69)

6e164a7

Lint with black (facebookresearch#69)

c5daa0c

Merge branch 'main' of https://github.com/SamDewriter/SimulEval into …

0279a59

…dynamic_language

Import from typing (facebookresearch#69)

72254a7

Lint (facebookresearch#69)

e3b90a9

Handle when tgt_lang is not for s2s (facebookresearch#69)

b28570e

SamDewriter requested a review from ibanesh September 20, 2023 16:45

ibanesh reviewed Sep 20, 2023

View reviewed changes

Mubaraq Sani added 3 commits September 20, 2023 19:50

Remove comments (facebookresearch#69)

dcc037d

Add check for tgt_lang s2t (facebookresearch#69)

c5e0010

Format with black (facebookresearch#69)

1698117

SamDewriter requested a review from ibanesh September 20, 2023 19:11

ibanesh approved these changes Sep 20, 2023

View reviewed changes

ibanesh merged commit b07447a into facebookresearch:main Sep 21, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to switch output languages for multilingual models #69 #74

Add ability to switch output languages for multilingual models #69 #74

SamDewriter commented Aug 28, 2023

ibanesh left a comment

ibanesh Sep 7, 2023

ibanesh Sep 20, 2023

ibanesh Sep 7, 2023

ibanesh Sep 7, 2023

ibanesh left a comment

ibanesh Sep 13, 2023

ibanesh Sep 13, 2023

SamDewriter Sep 17, 2023

ibanesh Sep 14, 2023

ibanesh Sep 14, 2023

ibanesh Sep 14, 2023

ibanesh Sep 14, 2023

ibanesh Sep 14, 2023

ibanesh Sep 14, 2023

ibanesh Sep 14, 2023

ibanesh Sep 14, 2023

ibanesh Sep 20, 2023

ibanesh left a comment

ibanesh Sep 20, 2023

SamDewriter Sep 20, 2023

ibanesh Sep 20, 2023

ibanesh left a comment

		def preprocess_tgt_lang(self, tgt_lang: Any) -> Any:
		raise NotImplementedError

	def get_tgt_lang(self, index: int) -> Any:
	def get_tgt_lang(self, index: int) -> Optional[str]:

		with open(tgt_lang) as f:
		tgt_lang_list = [line.strip() for line in f]

Add ability to switch output languages for multilingual models #69 #74

Add ability to switch output languages for multilingual models #69 #74

Conversation

SamDewriter commented Aug 28, 2023

ibanesh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibanesh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibanesh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibanesh left a comment

Choose a reason for hiding this comment