Fix leftover configuration from test_config.py (#1971)

Summary:
Pull Request resolved: #1971

Python tests on github failing with:
```
pyo3_runtime.PanicException: called `Result::unwrap()` on an `Err` value:
Server(Listen(Tcp([::1]:0),
Os { code: 99, kind: AddrNotAvailable, message: "Cannot assign requested address" }))
```
It is caused by the default transport for tests changing from unix -> tcp.
It's not clear what caused this change, but probably a new test was added that changed
the pytest-split groups.

Make sure test_config.py resets to the default state when it is done.

Reviewed By: rusch95, shayne-fletcher

Differential Revision: D87664159

fbshipit-source-id: d8d1279d7909cf487df27500ceba56bc93e820c0

Author: dulinriley

monarch_hyperactor/src/config.rs

@@ -47,6 +47,13 @@ pub fn reload_config_from_env() -> PyResult<()> {
     Ok(())
 }
 
+#[pyfunction()]
+pub fn reset_config_to_defaults() -> PyResult<()> {
+    // Set all config values to defaults, ignoring even environment variables.
+    hyperactor::config::global::reset_to_defaults();
+    Ok(())
+}
+
 /// Map from the kwarg name passed to `monarch.configure(...)` to the
 /// `Key<T>` associated with that kwarg. This contains all attribute
 /// keys whose `@meta(CONFIG = ConfigAttr { py_name: Some(...), .. })`
@@ -238,6 +245,13 @@ pub fn register_python_bindings(module: &Bound<'_, PyModule>) -> PyResult<()> {
     )?;
     module.add_function(reload)?;
 
+    let reset = wrap_pyfunction!(reset_config_to_defaults, module)?;
+    reset.setattr(
+        "__module__",
+        "monarch._rust_bindings.monarch_hyperactor.config",
+    )?;
+    module.add_function(reset)?;
+
     let configure = wrap_pyfunction!(configure, module)?;
     configure.setattr(
         "__module__",

python/monarch/_rust_bindings/monarch_hyperactor/config.pyi

@@ -20,13 +20,26 @@ def reload_config_from_env() -> None:
 
     This reads all HYPERACTOR_* environment variables and updates
     the global configuration.
+    For any configuration setting not present in environment variables,
+    this function will not change its value.
+    """
+    ...
+
+def reset_config_to_defaults() -> None:
+    """Reset all configuration to default values, ignoring environment variables.
+    Call reload_config_from_env() to reload the environment variables.
     """
     ...
 
 def configure(
-    default_transport: ChannelTransport = ChannelTransport.Unix,
-    enable_log_forwarding: bool = False,
-    enable_file_capture: bool = False,
-    tail_log_lines: int = 0,
-) -> None: ...
+    default_transport: ChannelTransport = ...,
+    enable_log_forwarding: bool = ...,
+    enable_file_capture: bool = ...,
+    tail_log_lines: int = ...,
+    **kwargs: object,
+) -> None:
+    """Change a configuration value in the global configuration. If called with
+    no arguments, makes no changes. Does not reset any configuration"""
+    ...
+
 def get_configuration() -> Dict[str, Any]: ...

python/tests/test_config.py

@@ -6,47 +6,76 @@
 
 # pyre-unsafe
 
+from contextlib import contextmanager
+
 import pytest
 from monarch._rust_bindings.monarch_hyperactor.channel import ChannelTransport
 from monarch._rust_bindings.monarch_hyperactor.config import (
     configure,
     get_configuration,
+    reload_config_from_env,
+    reset_config_to_defaults,
 )
 
 
+@contextmanager
+def configure_temporary(*args, **kwargs):
+    """Call configure, and then reset the configuration to the default values after
+    exiting. Always use this when testing so that other tests are not affected by any
+    changes made."""
+
+    try:
+        configure(*args, **kwargs)
+        yield
+    finally:
+        reset_config_to_defaults()
+        # Re-apply any environment variables that were set for this test.
+        reload_config_from_env()
+
+
 def test_get_set_transport() -> None:
     for transport in (
         ChannelTransport.Unix,
         ChannelTransport.TcpWithLocalhost,
         ChannelTransport.TcpWithHostname,
         ChannelTransport.MetaTlsWithHostname,
     ):
-        configure(default_transport=transport)
-        assert get_configuration()["default_transport"] == transport
-    # Succeed even if we don't specify the transport
-    configure()
-    assert (
-        get_configuration()["default_transport"] == ChannelTransport.MetaTlsWithHostname
-    )
+        with configure_temporary(default_transport=transport):
+            assert get_configuration()["default_transport"] == transport
+    # Succeed even if we don't specify the transport, but does not change the
+    # previous value.
+    with configure_temporary():
+        assert get_configuration()["default_transport"] == ChannelTransport.Unix
     with pytest.raises(TypeError):
-        configure(default_transport="unix")  # type: ignore
+        with configure_temporary(default_transport="unix"):  # type: ignore
+            pass
     with pytest.raises(TypeError):
-        configure(default_transport=42)  # type: ignore
+        with configure_temporary(default_transport=42):  # type: ignore
+            pass
     with pytest.raises(TypeError):
-        configure(default_transport={})  # type: ignore
+        with configure_temporary(default_transport={}):  # type: ignore
+            pass
 
 
 def test_nonexistent_config_key() -> None:
     with pytest.raises(ValueError):
-        configure(does_not_exist=42)  # type: ignore
+        with configure_temporary(does_not_exist=42):  # type: ignore
+            pass
 
 
 def test_get_set_multiple() -> None:
-    configure(default_transport=ChannelTransport.TcpWithLocalhost)
-    configure(enable_log_forwarding=True, enable_file_capture=True, tail_log_lines=100)
+    with configure_temporary(default_transport=ChannelTransport.TcpWithLocalhost):
+        with configure_temporary(
+            enable_log_forwarding=True, enable_file_capture=True, tail_log_lines=100
+        ):
+            config = get_configuration()
+            assert config["enable_log_forwarding"]
+            assert config["enable_file_capture"]
+            assert config["tail_log_lines"] == 100
+            assert config["default_transport"] == ChannelTransport.TcpWithLocalhost
+    # Make sure the previous values are restored.
     config = get_configuration()
-
-    assert config["enable_log_forwarding"]
-    assert config["enable_file_capture"]
-    assert config["tail_log_lines"] == 100
-    assert config["default_transport"] == ChannelTransport.TcpWithLocalhost
+    assert not config["enable_log_forwarding"]
+    assert not config["enable_file_capture"]
+    assert config["tail_log_lines"] == 0
+    assert config["default_transport"] == ChannelTransport.Unix

scripts/common-setup.sh

@@ -179,6 +179,8 @@ run_test_groups() {
   # sustainable/most robust solution.
   export CONDA_LIBSTDCPP="${CONDA_PREFIX}/lib/libstdc++.so.6"
   export LD_PRELOAD="${CONDA_LIBSTDCPP}${LD_PRELOAD:+:$LD_PRELOAD}"
+  # Backtraces help with debugging remotely.
+  export RUST_BACKTRACE=1
   local FAILED_GROUPS=()
   for GROUP in $(seq 1 10); do
     echo "Running test group $GROUP of 10..."