Add option to generate LM image and GC via two separate jobs #446

NeoLegends · 2023-08-21T16:34:02Z

Closes #430
Closes #514

Now testing...

Closes #430

recognition/advanced_tree_search.py

michelwi · 2023-08-22T07:53:31Z

recognition/advanced_tree_search.py

+        if separate_lmi_gc_generation:
+            gc = BuildGlobalCacheJob(crp, extra_config, extra_post_config).out_global_cache
+
+            arpa_lms = AdvancedTreeSearchLmImageAndGlobalCacheJob.find_arpa_lms(


maybe find_arpa_lms can be moved from a classmethod to a standalone method in the lm module?

See rwth-i6/rasr@d58a228

NeoLegends · 2023-08-29T15:26:51Z

I just discovered during testing the LM image and GC are set on the post config, not in the normal config. This means splitting the LMGC-Job into two does not change the hash of any existing jobs.

Therefore, I think this flag can be enabled by default, or rather, we can in-line the flag and make it the new default! WDYT?

To finish testing I need to run a decoding w/ the new GC and new LM, but otherwise the stuff here is now tested. This is what it looks like when the flag is set in a pipeline w/ thousands of jobs (note due to the GC not being hashed only a few GC-jobs are even run):

[2023-08-29 17:30:46,918] INFO: Finished updating job states
[2023-08-29 17:30:46,934] INFO: Experiment directory: /u/mgunz/setups/2023-08--subsampling-new      Call: /u/mgunz/src/sisyphus/sis m -r -io
[2023-08-29 17:30:47,002] INFO: runnable: Job<work/i6_core/lm/lm_image/CreateLmImageJob.gNAo29L3jB0Y>
[2023-08-29 17:30:47,002] INFO: runnable: Job<work/i6_core/recognition/advanced_tree_search/BuildGlobalCacheJob.9kZVSb7jUVMk>
[2023-08-29 17:30:47,002] INFO: runnable: Job<work/i6_core/recognition/advanced_tree_search/BuildGlobalCacheJob.VesmsCzSguWG>
[2023-08-29 17:30:47,002] INFO: runnable(3) running(8) waiting(878)

NeoLegends · 2023-08-29T15:36:48Z

Apparently switching the default here changes hashes at AppTek. In case it is possible to live w/ the hash breakage (e.g. because it is in unused parts of the code) I'd like the flag to be on by default so as many folks as possible can profit from the changes here. If the hash breakage is unacceptable we leave it off by default of course.

michelwi · 2023-08-30T12:37:22Z

I just discovered during testing the LM image and GC are set on the post config, not in the normal config. This means splitting the LMGC-Job into two does not change the hash of any existing jobs.

*Any existing search jobs. The graph however is changed as in all of the LMGC Jobs are now removed and separate LM and GC Jobs are added. This is caught in the pipeline.

In case it is possible to live w/ the hash breakage (e.g. because it is in unused parts of the code) I'd like the flag to be on by default so as many folks as possible can profit from the changes here. If the hash breakage is unacceptable we leave it off by default of course.

Unfortunately all parts that are tested in the pipeline are used.

lm/lm_image.py

rasr/crp.py

recognition/advanced_tree_search.py

NeoLegends · 2023-08-31T09:10:39Z

I have successfully run recognitions w/ this setup. This is tested now.

recognition/advanced_tree_search.py

JackTemaki · 2023-09-14T21:30:28Z

recognition/advanced_tree_search.py

@@ -190,6 +173,7 @@ def __init__(
        :param lmgc_mem: Memory requirement for the AdvancedTreeSearchLmImageAndGlobalCacheJob
        :param lmgc_alias: Alias for the AdvancedTreeSearchLmImageAndGlobalCacheJob
        :param lmgc_scorer: Dummy scorer for the AdvancedTreeSearchLmImageAndGlobalCacheJob which is required but unused
+        :param separate_lm_image_gc_generation: Whether to generate the LM image and the global cache via two separate jobs for a more stable hash. Whether or not this flag is set is not part of the hash, so using separate jobs is the default.


Shouldn't this be a so NOT using separate jobs is the default?

NeoLegends · 2023-11-06T12:55:52Z

AppTek hash broke after changing only a comment, did the AppTek pipeline change in the meantime?

Marvin84 · 2023-11-06T12:57:27Z

AppTek hash broke after changing only a comment, did the AppTek pipeline change in the meantime?

@curufinwe @michelwi @christophmluscher any idea?

michelwi · 2023-11-08T13:05:29Z

AppTek hash broke

We now do require the changes that were introduced in #455 and your PR is so old it does not include them. If you rebase to the current main all should be fine. Thanks.

lm/util.py

Marvin84 · 2023-11-14T16:13:00Z

@curufinwe @michelwi can we merge this?

michelwi · 2023-11-14T19:10:58Z

@curufinwe @michelwi can we merge this?

Since the apptek test passes, I see no objections from our site.

I'll dismiss my old review, but I currently don't have much time to re-review it.. sorry.

No time to follow up on it.

Atticus1806 · 2023-11-15T14:40:22Z

lm/util.py

+import i6_core.rasr as rasr
+
+
+def _has_image(c: rasr.RasrConfig, pc: rasr.RasrConfig):


I think I would either like to spell config and post_config out or have a small docstring here.

I agree with @Atticus1806

Atticus1806 · 2023-11-15T14:40:56Z

lm/util.py

+    return res
+
+
+def find_arpa_lms(lm_config: rasr.RasrConfig, lm_post_config=None) -> List[Tuple[rasr.RasrConfig, rasr.RasrConfig]]:


Is the post config also rasr.RasrConfig? Or some other type?

Suggested change

def find_arpa_lms(lm_config: rasr.RasrConfig, lm_post_config=None) -> List[Tuple[rasr.RasrConfig, rasr.RasrConfig]]:

def find_arpa_lms(lm_config: rasr.RasrConfig, lm_post_config: Optional[rasr.RasrConfig] = None) -> List[Tuple[rasr.RasrConfig, rasr.RasrConfig]]:

yes, post configs should also be of type RasrConfig
TODO: add Optional import above and check with black for line length :P

Atticus1806 · 2023-11-15T14:47:50Z

recognition/advanced_tree_search.py

@@ -206,6 +190,8 @@ def __init__(
            self.config,
            self.post_config,
            self.lm_gc_job,
+            self.gc_job,
+            self.lm_image_jobs,
        ) = AdvancedTreeSearchJob.create_config(**kwargs)


Is .create_config only called within this job? Otherwise we need to be careful with changing the returned variables. But I think you caught the cases here and otherwise the change is easy.

the AdvancedTreeSearchWithRescoringJob below in this file inherits from AdvancedTreeSearchJob and uses super().create_config. that should be fixed.

Atticus1806 · 2023-11-15T14:50:28Z

recognition/advanced_tree_search.py

-        if lmgc_alias is not None:
-            lm_gc.add_alias(lmgc_alias)
-        lm_gc.rqmt["mem"] = lmgc_mem
+        def add_lm_config_to_crp(crp, lm_config):


Suggested change

def add_lm_config_to_crp(crp, lm_config):

def add_lm_config_to_crp(crp: rasr.CommonRasrParameters, lm_config: rasr.RasrConfig):

recognition/advanced_tree_search.py

Atticus1806 · 2023-11-15T14:54:23Z

recognition/advanced_tree_search.py

            config.flf_lattice_tool.network.recognizer.lm,
            post_config.flf_lattice_tool.network.recognizer.lm,
        )
-        for i, lm_config in enumerate(arpa_lms):
-            lm_config[1].image = lm_gc.out_lm_images[i + 1]
+        for i, (_lm_config, lm_post_config) in enumerate(arpa_lms):


Same as above

Atticus1806 · 2023-11-15T14:54:55Z

recognition/advanced_tree_search.py

+                (i + 1): lm.CreateLmImageJob(
+                    add_lm_config_to_crp(crp, lm_config), extra_config=extra_config, extra_post_config=extra_post_config
+                )
+                for i, (lm_config, _lm_post_config) in enumerate(arpa_lms)


Is there a reason for the _. Maybe I am overlooking something in the web view. If its not used you could just fully replace it with _

I think the `post_config here is unused, because

both config and post_confic are extracted from the crp

only the config is modified

the crp with the old post_config is still being used

+1 to can be just _ to make clear it is unused

Suggested change

for i, (lm_config, _lm_post_config) in enumerate(arpa_lms)

for i, (lm_config, _) in enumerate(arpa_lms)

recognition/advanced_tree_search.py

Atticus1806 · 2023-11-15T14:58:50Z

can we merge this?

With two approves yes :). I just got a few comments then I can approve, but please ask someone else to also review.

michelwi

I think many open comments should be addressed before merging.

michelwi · 2023-12-04T08:52:07Z

lm/util.py

+import i6_core.rasr as rasr
+
+
+def _has_image(c: rasr.RasrConfig, pc: rasr.RasrConfig):


I agree with @Atticus1806

michelwi · 2023-12-04T08:53:11Z

lm/util.py

+    return res
+
+
+def find_arpa_lms(lm_config: rasr.RasrConfig, lm_post_config=None) -> List[Tuple[rasr.RasrConfig, rasr.RasrConfig]]:


Suggested change

def find_arpa_lms(lm_config: rasr.RasrConfig, lm_post_config=None) -> List[Tuple[rasr.RasrConfig, rasr.RasrConfig]]:

def find_arpa_lms(lm_config: rasr.RasrConfig, lm_post_config: Optional[rasr.RasrConfig] = None) -> List[Tuple[rasr.RasrConfig, rasr.RasrConfig]]:

yes, post configs should also be of type RasrConfig
TODO: add Optional import above and check with black for line length :P

recognition/advanced_tree_search.py

michelwi · 2023-12-04T09:05:03Z

recognition/advanced_tree_search.py

+                (i + 1): lm.CreateLmImageJob(
+                    add_lm_config_to_crp(crp, lm_config), extra_config=extra_config, extra_post_config=extra_post_config
+                )
+                for i, (lm_config, _lm_post_config) in enumerate(arpa_lms)


I think the `post_config here is unused, because

both config and post_confic are extracted from the crp

only the config is modified

the crp with the old post_config is still being used

+1 to can be just _ to make clear it is unused

Suggested change

for i, (lm_config, _lm_post_config) in enumerate(arpa_lms)

for i, (lm_config, _) in enumerate(arpa_lms)

michelwi · 2023-12-04T09:05:26Z

recognition/advanced_tree_search.py

+                (i + 1): lm.CreateLmImageJob(
+                    add_lm_config_to_crp(crp, lm_config), extra_config=extra_config, extra_post_config=extra_post_config
+                )
+                for i, (lm_config, _lm_post_config) in enumerate(arpa_lms)


I think the `post_config here is unused, because

both config and post_confic are extracted from the crp

only the config is modified

the crp with the old post_config is still being used

+1 to can be just _ to make clear it is unused

Suggested change

for i, (lm_config, _lm_post_config) in enumerate(arpa_lms)

for i, (lm_config, _) in enumerate(arpa_lms)

recognition/advanced_tree_search.py

michelwi · 2024-06-19T08:50:14Z

recognition/advanced_tree_search.py

@@ -190,6 +173,7 @@ def __init__(
        :param lmgc_mem: Memory requirement for the AdvancedTreeSearchLmImageAndGlobalCacheJob
        :param lmgc_alias: Alias for the AdvancedTreeSearchLmImageAndGlobalCacheJob
        :param lmgc_scorer: Dummy scorer for the AdvancedTreeSearchLmImageAndGlobalCacheJob which is required but unused
+        :param separate_lm_image_gc_generation: Whether to generate the LM image and the global cache via two separate jobs for a more stable hash. Whether or not this flag is set is not part of the hash, so NOT using separate jobs is the default.


Suggested change

:param separate_lm_image_gc_generation: Whether to generate the LM image and the global cache via two separate jobs for a more stable hash. Whether or not this flag is set is not part of the hash, so NOT using separate jobs is the default.

:param separate_lm_image_gc_generation: Whether to generate the LM image and the global cache via two separate

jobs for a more stable hash. This flag is not part of the hash, using a combined job is the default.

break comment into two lines to respect 120 char line length?

michelwi · 2024-06-19T09:04:38Z

recognition/advanced_tree_search.py

+        if separate_lm_image_gc_generation:
+            gc_job = BuildGlobalCacheJob(crp, extra_config, extra_post_config)
+
+            arpa_lms = lm.find_arpa_lms(crp.language_model_config, None)


The function lm.find_arpa_lms only returns the LMs that do not already have an lm.image - because for those that already have an image we do not need to create a new one.

But the lm.image usually is defined in the post_config. When we here not pass the post config, then all (arpa) LMs are found and returned. Therefore we do extra work here.
And even worse: below in line 404/418 we call the function again, but with the post config. So if the original crp contain a mix of arpa LMs out of which some have already images and others do not, then the items in arpa_lms differ between the calls. And since we use the index to match the image to the LM, the mapping will be off and the wrong image will be assigned.

Suggested change

arpa_lms = lm.find_arpa_lms(crp.language_model_config, None)

arpa_lms = lm.find_arpa_lms(crp.language_model_config, crp.language_model_post_config)

michelwi · 2024-06-19T09:07:50Z

recognition/advanced_tree_search.py


-        arpa_lms = AdvancedTreeSearchLmImageAndGlobalCacheJob.find_arpa_lms(
+        arpa_lms = lm.find_arpa_lms(


We already have called lm.find_arpa_lms above. Maybe move the above call out of the if condition and then reuse the arpa_lms from above here. This would also avoid mismatching items in the list as I outlined above.

michelwi · 2024-06-19T09:10:36Z

recognition/advanced_tree_search.py

@@ -206,6 +190,8 @@ def __init__(
            self.config,
            self.post_config,
            self.lm_gc_job,
+            self.gc_job,
+            self.lm_image_jobs,
        ) = AdvancedTreeSearchJob.create_config(**kwargs)


the AdvancedTreeSearchWithRescoringJob below in this file inherits from AdvancedTreeSearchJob and uses super().create_config. that should be fixed.

feat: Add option to generate LM image and GC via two separate jobs

620a042

Closes #430

NeoLegends added the enhancement New feature or request label Aug 21, 2023

NeoLegends requested review from christophmluscher and JackTemaki August 21, 2023 16:34

NeoLegends self-assigned this Aug 21, 2023

NeoLegends marked this pull request as draft August 21, 2023 16:34

NeoLegends added 2 commits August 21, 2023 18:34

chore: Document parameter

a68265c

fix: Always assign the (possibly to None) lm_gc property

b74c654

NeoLegends force-pushed the feat/separate-lmi-gc-generation branch from 6b40167 to b74c654 Compare August 21, 2023 16:39

michelwi previously requested changes Aug 22, 2023

View reviewed changes

NeoLegends added 5 commits August 22, 2023 10:22

fix bug, assign jobs to class if possible

79cbd2a

refactor find_arpa_lms into standalone function

78f6bed

Merge branch 'main' into feat/separate-lmi-gc-generation

291b734

fix bugs from trial runs

d209e8f

Re-enable lm-util

02d7088

See rwth-i6/rasr@d58a228

NeoLegends requested a review from michelwi August 29, 2023 14:44

NeoLegends added 2 commits August 29, 2023 16:48

more mem for LM + GC jobs

6a236c8

make mem configurable

a61b03f

NeoLegends marked this pull request as ready for review August 29, 2023 14:49

even more mem

008c9dc

enable split behavior by default, document hash implications

07e1136

NeoLegends changed the title ~~feat: Add option to generate LM image and GC via two separate jobs~~ Add option to generate LM image and GC via two separate jobs Aug 29, 2023

disable flag by default

cdc791a

michelwi reviewed Aug 30, 2023

View reviewed changes

lm/lm_image.py Show resolved Hide resolved

rasr/crp.py Show resolved Hide resolved

recognition/advanced_tree_search.py Show resolved Hide resolved

recognition/advanced_tree_search.py Show resolved Hide resolved

JackTemaki reviewed Sep 6, 2023

View reviewed changes

recognition/advanced_tree_search.py Outdated Show resolved Hide resolved

recognition/advanced_tree_search.py Outdated Show resolved Hide resolved

Rename flag to be more clear

ab010c5

rename local function

5a8ec3d

NeoLegends requested review from michelwi and JackTemaki September 11, 2023 15:31

JackTemaki reviewed Sep 14, 2023

View reviewed changes

fix wording

ed001c4

Marvin84 requested review from curufinwe, Marvin84 and Atticus1806 November 6, 2023 12:32

Merge branch 'main' into feat/separate-lmi-gc-generation

115b160

Atticus1806 reviewed Nov 9, 2023

View reviewed changes

lm/util.py Show resolved Hide resolved

Atticus1806 reviewed Nov 15, 2023

View reviewed changes

michelwi reviewed Dec 4, 2023

View reviewed changes

JackTemaki mentioned this pull request Jun 18, 2024

Exclusion of unnecessary parameters from AdvancedTreeSearchLmImageAndGlobalCacheJob hash calculation #514

Open

NeoLegends removed their assignment Jun 18, 2024

michelwi requested changes Jun 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to generate LM image and GC via two separate jobs #446

Add option to generate LM image and GC via two separate jobs #446

NeoLegends commented Aug 21, 2023 •

edited

Loading

michelwi Aug 22, 2023

NeoLegends commented Aug 29, 2023 •

edited

Loading

NeoLegends commented Aug 29, 2023

michelwi commented Aug 30, 2023

NeoLegends commented Aug 31, 2023

JackTemaki Sep 14, 2023

NeoLegends commented Nov 6, 2023

Marvin84 commented Nov 6, 2023

michelwi commented Nov 8, 2023

Marvin84 commented Nov 14, 2023

michelwi commented Nov 14, 2023

Atticus1806 Nov 15, 2023

Marvin84 Nov 16, 2023

michelwi Dec 4, 2023

Atticus1806 Nov 15, 2023

michelwi Dec 4, 2023

Atticus1806 Nov 15, 2023

michelwi Jun 19, 2024

Atticus1806 Nov 15, 2023

Atticus1806 Nov 15, 2023

Atticus1806 Nov 15, 2023

michelwi Dec 4, 2023

michelwi Dec 4, 2023

Atticus1806 commented Nov 15, 2023

michelwi left a comment

michelwi Dec 4, 2023

michelwi Dec 4, 2023

michelwi Dec 4, 2023

michelwi Dec 4, 2023

michelwi Jun 19, 2024

michelwi Jun 19, 2024

michelwi Jun 19, 2024

michelwi Jun 19, 2024

		import i6_core.rasr as rasr


		def _has_image(c: rasr.RasrConfig, pc: rasr.RasrConfig):

		return res


		def find_arpa_lms(lm_config: rasr.RasrConfig, lm_post_config=None) -> List[Tuple[rasr.RasrConfig, rasr.RasrConfig]]:

	def add_lm_config_to_crp(crp, lm_config):
	def add_lm_config_to_crp(crp: rasr.CommonRasrParameters, lm_config: rasr.RasrConfig):

	for i, (lm_config, _lm_post_config) in enumerate(arpa_lms)
	for i, (lm_config, _) in enumerate(arpa_lms)

	:param separate_lm_image_gc_generation: Whether to generate the LM image and the global cache via two separate jobs for a more stable hash. Whether or not this flag is set is not part of the hash, so NOT using separate jobs is the default.
	:param separate_lm_image_gc_generation: Whether to generate the LM image and the global cache via two separate
	jobs for a more stable hash. This flag is not part of the hash, using a combined job is the default.

	arpa_lms = lm.find_arpa_lms(crp.language_model_config, None)
	arpa_lms = lm.find_arpa_lms(crp.language_model_config, crp.language_model_post_config)


		arpa_lms = AdvancedTreeSearchLmImageAndGlobalCacheJob.find_arpa_lms(
		arpa_lms = lm.find_arpa_lms(

Add option to generate LM image and GC via two separate jobs #446

Are you sure you want to change the base?

Add option to generate LM image and GC via two separate jobs #446

Conversation

NeoLegends commented Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

NeoLegends commented Aug 29, 2023 • edited Loading

NeoLegends commented Aug 29, 2023

michelwi commented Aug 30, 2023

NeoLegends commented Aug 31, 2023

Choose a reason for hiding this comment

NeoLegends commented Nov 6, 2023

Marvin84 commented Nov 6, 2023

michelwi commented Nov 8, 2023

Marvin84 commented Nov 14, 2023

michelwi commented Nov 14, 2023

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

Atticus1806 commented Nov 15, 2023

michelwi 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

NeoLegends commented Aug 21, 2023 •

edited

Loading

NeoLegends commented Aug 29, 2023 •

edited

Loading