Change override to override_sole_init in alf.config #1705

QuantuMope · 2024-09-20T00:40:00Z

This PR changes the override flag of alf.config to override_sole_init. The implementation before allowed alf.override_config to override a value of a config regardless of the mutable flag. Unfortunately, this breaks the logic of setting parameters via --conf_params or alf.pre_config. It also causes issues in evaluator processes that may set certain config values with mutable=False and then read from the conf file as it overwrites the desired eval setting.

Therefore, this PR now changes alf.override_config to alf.override_sole_config, which respects immutability. On our local hobot code, I've now verified that all the above problems have been remedied and singular config calls can now be enforced when using ALF_SOLE_CONFIG=1, greatly reducing chance for bugs via nested / unintentional overwrites.

Additional test cases for edge cases has also been provided in config_utils_test.py.

Once this gets merged, I can submit PRs for ALF_SOLE_CONFIG=1 compatible hobot confs.

le-horizon

Thanks for the much cleaner changes and tests, Andrew. A few questions regarding alf config behavior.

Le

le-horizon · 2024-09-20T21:35:59Z

alf/config_util_test.py

@@ -182,6 +182,63 @@ def sole_init_test_env(x):
        self.assertEqual(alf.get_config_value("sole_init_test_twice.x"), 1)
        self.assertEqual(alf.get_config_value("sole_init_test_env.x"), 1)

+        # Test override_config doesn't doesn't overwrite for immutable values.


remove one "doesn't"

Thanks! Done.

le-horizon · 2024-09-20T21:38:02Z

alf/config_util_test.py

+            pass
+
+        alf.config("override_on_immutable", x=0, mutable=False)
+        alf.override_config("override_on_immutable", x=1)


Is it possible to raise an error on overriding immutable config? Silently ignoring override can be hard to debug?

Refer to my reply above for why this cannot be done currently without adding more state parameters. There is a logger warning that gets printed in this scenario, though it's easy for users to not see it.

Maybe let's color code the warning message a bright yellow, so it's harder to miss?

Right now it prints as red via absl.logging.warning. Do you think yellow is better?

alf/config_util_test.py

le-horizon · 2024-09-20T21:53:21Z

alf/config_util_test.py

+            pass
+
+        alf.config("override_no_affect_sole_init", x=1, sole_init=True)
+        alf.override_config("override_no_affect_sole_init", x=2)


should this also throw error?

It shouldn't throw an error since override_config allows us to set a config's value despite an earlier sole_init=True call.

Oh, I see. Maybe rename the function to alf.override_sole_config()? Just as how you renamed override to override_sole_init.

Good idea. Done.

alf/config_util.py

le-horizon · 2024-09-20T21:57:07Z

alf/config_util.py

+            only when absolutely necessary (e.g., a teacher-student training loop,
+            where the student must override certain configs inherited from the
+            teacher). If the config is immutable, a warning will be declared with
+            no changes made.


instead of quietly ignoring the config, should we just throw error for this case?

So the reason we cannot do this is because alf.pre_config and --conf_param set values with mutable=False (that is how those values avoid getting overwritten). Therefore, there may be times we want to override our conf files using one of the two options and raising an error in this situation is problematic. A workaround for this was to possibly introduce a pre_configged state variable for configs, but I thought that may be unnecessary. Let me know what you think.

I see. You are right. Let's go with current way for the PR.

Separately, are there some examples of these cases? It would be good to understand a bit more, and see if we can design better solutions.

Are you referring to examples of the teacher-student setup?

One is our own agent training. Let's say you train expert with ALF_SOLE_CONFIG=1. When we train the agent with ALF_SOLE_CONFIG=1 as well, it will inevitably raise errors as agent often overwrites some of the expert's configs. Here, we can use the alf.override_config function. Not only does this minimize config calls, but it also explicitly tells the coder which inherited calls are being overwritten.

QuantuMope

Thanks for the really speedy review Le. Addressed your comments. Let me know if there are any questions or clarifications I can make.

alf/config_util.py

QuantuMope · 2024-09-20T22:09:46Z

alf/config_util.py

+            only when absolutely necessary (e.g., a teacher-student training loop,
+            where the student must override certain configs inherited from the
+            teacher). If the config is immutable, a warning will be declared with
+            no changes made.


So the reason we cannot do this is because alf.pre_config and --conf_param set values with mutable=False (that is how those values avoid getting overwritten). Therefore, there may be times we want to override our conf files using one of the two options and raising an error in this situation is problematic. A workaround for this was to possibly introduce a pre_configged state variable for configs, but I thought that may be unnecessary. Let me know what you think.

QuantuMope · 2024-09-20T22:10:28Z

alf/config_util_test.py

@@ -182,6 +182,63 @@ def sole_init_test_env(x):
        self.assertEqual(alf.get_config_value("sole_init_test_twice.x"), 1)
        self.assertEqual(alf.get_config_value("sole_init_test_env.x"), 1)

+        # Test override_config doesn't doesn't overwrite for immutable values.


Thanks! Done.

QuantuMope · 2024-09-20T22:11:35Z

alf/config_util_test.py

+            pass
+
+        alf.config("override_on_immutable", x=0, mutable=False)
+        alf.override_config("override_on_immutable", x=1)


Refer to my reply above for why this cannot be done currently without adding more state parameters. There is a logger warning that gets printed in this scenario, though it's easy for users to not see it.

alf/config_util_test.py

QuantuMope · 2024-09-20T22:13:15Z

alf/config_util_test.py

+            pass
+
+        alf.config("override_no_affect_sole_init", x=1, sole_init=True)
+        alf.override_config("override_no_affect_sole_init", x=2)


It shouldn't throw an error since override_config allows us to set a config's value despite an earlier sole_init=True call.

le-horizon

Thanks for answering the questions. Only a few nits.

Thanks,
Le

le-horizon · 2024-09-23T18:14:53Z

alf/config_util.py

+            only when absolutely necessary (e.g., a teacher-student training loop,
+            where the student must override certain configs inherited from the
+            teacher). If the config is immutable, a warning will be declared with
+            no changes made.


I see. You are right. Let's go with current way for the PR.

Separately, are there some examples of these cases? It would be good to understand a bit more, and see if we can design better solutions.

le-horizon · 2024-09-23T18:15:48Z

alf/config_util_test.py

+            pass
+
+        alf.config("override_on_immutable", x=0, mutable=False)
+        alf.override_config("override_on_immutable", x=1)


Maybe let's color code the warning message a bright yellow, so it's harder to miss?

alf/config_util_test.py

le-horizon · 2024-09-23T18:20:56Z

alf/config_util_test.py

+        alf.config("override_no_affect_sole_init", x=1, sole_init=True)
+        alf.override_config("override_no_affect_sole_init", x=2)
+        with self.assertRaises(RuntimeError) as context:
+            alf.config("override_no_affect_sole_init", x=3)


test that x's value is 2

Thanks. Done.

le-horizon · 2024-09-23T18:22:49Z

alf/config_util_test.py

+            pass
+
+        alf.config("override_no_affect_sole_init", x=1, sole_init=True)
+        alf.override_config("override_no_affect_sole_init", x=2)


Oh, I see. Maybe rename the function to alf.override_sole_config()? Just as how you renamed override to override_sole_init.

emailweixu · 2024-09-23T19:38:29Z

alf/config_helpers.py

@@ -285,7 +285,11 @@ def get_env():
            # A 'None' random seed won't set a deterministic torch behavior.
            for _ in range(PerProcessContext().ddp_rank):
                random_seed = random.randint(0, 2**32)
-        config1("TrainerConfig.random_seed", random_seed, raise_if_used=False)
+        config1(


there are several other config1 calls in config_helpers.py that may need fix too.

Thanks for the catch. This has been fixed for the other calls.

QuantuMope

Thanks for the comments. PTAL

QuantuMope · 2024-09-24T17:56:31Z

alf/config_helpers.py

@@ -285,7 +285,11 @@ def get_env():
            # A 'None' random seed won't set a deterministic torch behavior.
            for _ in range(PerProcessContext().ddp_rank):
                random_seed = random.randint(0, 2**32)
-        config1("TrainerConfig.random_seed", random_seed, raise_if_used=False)
+        config1(


Thanks for the catch. This has been fixed for the other calls.

QuantuMope · 2024-09-24T17:59:32Z

alf/config_util.py

+            only when absolutely necessary (e.g., a teacher-student training loop,
+            where the student must override certain configs inherited from the
+            teacher). If the config is immutable, a warning will be declared with
+            no changes made.


Are you referring to examples of the teacher-student setup?

One is our own agent training. Let's say you train expert with ALF_SOLE_CONFIG=1. When we train the agent with ALF_SOLE_CONFIG=1 as well, it will inevitably raise errors as agent often overwrites some of the expert's configs. Here, we can use the alf.override_config function. Not only does this minimize config calls, but it also explicitly tells the coder which inherited calls are being overwritten.

QuantuMope · 2024-09-24T18:02:28Z

alf/config_util_test.py

+            pass
+
+        alf.config("override_on_immutable", x=0, mutable=False)
+        alf.override_config("override_on_immutable", x=1)


Right now it prints as red via absl.logging.warning. Do you think yellow is better?

QuantuMope · 2024-09-24T18:02:46Z

alf/config_util_test.py

+            pass
+
+        alf.config("override_no_affect_sole_init", x=1, sole_init=True)
+        alf.override_config("override_no_affect_sole_init", x=2)


Good idea. Done.

QuantuMope · 2024-09-24T18:04:58Z

alf/config_util_test.py

+        alf.config("override_no_affect_sole_init", x=1, sole_init=True)
+        alf.override_config("override_no_affect_sole_init", x=2)
+        with self.assertRaises(RuntimeError) as context:
+            alf.config("override_no_affect_sole_init", x=3)


Thanks. Done.

* Rename alf.override_config to alf.override_sole_config

QuantuMope requested review from emailweixu and hnyu September 20, 2024 00:40

QuantuMope force-pushed the PR/andrew/fix-sole-init branch from cbb6a84 to 7fe31a7 Compare September 20, 2024 00:42

Change override to override_sole_init in alf.config

bca872b

QuantuMope force-pushed the PR/andrew/fix-sole-init branch from 7fe31a7 to bca872b Compare September 20, 2024 00:44

QuantuMope requested a review from le-horizon September 20, 2024 21:29

le-horizon reviewed Sep 20, 2024

View reviewed changes

QuantuMope commented Sep 20, 2024

View reviewed changes

fix double doesn't

fadd4ef

le-horizon reviewed Sep 23, 2024

View reviewed changes

emailweixu reviewed Sep 23, 2024

View reviewed changes

QuantuMope commented Sep 24, 2024

View reviewed changes

Address comments

0da269c

QuantuMope force-pushed the PR/andrew/fix-sole-init branch from edb31e1 to 0da269c Compare September 24, 2024 18:09

emailweixu approved these changes Sep 24, 2024

View reviewed changes

le-horizon approved these changes Sep 25, 2024

View reviewed changes

QuantuMope merged commit 3487f59 into pytorch Sep 25, 2024
2 checks passed

QuantuMope deleted the PR/andrew/fix-sole-init branch September 25, 2024 23:31

QuantuMope added a commit that referenced this pull request Oct 2, 2024

Change override to override_sole_init in alf.config (#1705)

83901d4

* Rename alf.override_config to alf.override_sole_config

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change override to override_sole_init in alf.config #1705

Change override to override_sole_init in alf.config #1705

QuantuMope commented Sep 20, 2024 •

edited

Loading

le-horizon left a comment

le-horizon Sep 20, 2024

QuantuMope Sep 20, 2024

le-horizon Sep 20, 2024

QuantuMope Sep 20, 2024 •

edited

Loading

le-horizon Sep 23, 2024

QuantuMope Sep 24, 2024

le-horizon Sep 20, 2024

QuantuMope Sep 20, 2024

le-horizon Sep 23, 2024

QuantuMope Sep 24, 2024

le-horizon Sep 20, 2024

QuantuMope Sep 20, 2024

le-horizon Sep 23, 2024

QuantuMope Sep 24, 2024

QuantuMope left a comment

QuantuMope Sep 20, 2024

QuantuMope Sep 20, 2024

QuantuMope Sep 20, 2024 •

edited

Loading

QuantuMope Sep 20, 2024

le-horizon left a comment

le-horizon Sep 23, 2024

le-horizon Sep 23, 2024

le-horizon Sep 23, 2024

QuantuMope Sep 24, 2024

le-horizon Sep 23, 2024

emailweixu Sep 23, 2024

QuantuMope Sep 24, 2024

QuantuMope left a comment

QuantuMope Sep 24, 2024

QuantuMope Sep 24, 2024

QuantuMope Sep 24, 2024

QuantuMope Sep 24, 2024

QuantuMope Sep 24, 2024

Change override to override_sole_init in alf.config #1705

Change override to override_sole_init in alf.config #1705

Conversation

QuantuMope commented Sep 20, 2024 • edited Loading

le-horizon 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

QuantuMope Sep 20, 2024 • edited Loading

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

QuantuMope left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QuantuMope Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

le-horizon 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

QuantuMope 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

QuantuMope commented Sep 20, 2024 •

edited

Loading

QuantuMope Sep 20, 2024 •

edited

Loading

QuantuMope Sep 20, 2024 •

edited

Loading