From 8beffe1318324a03f5abe204039b8d73b6b3c228 Mon Sep 17 00:00:00 2001 From: Riley Dulin Date: Mon, 24 Nov 2025 09:00:18 -0800 Subject: [PATCH] Fix leftover configuration from test_config.py (#1971) Summary: 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 --- monarch_hyperactor/src/config.rs | 14 ++++ .../monarch_hyperactor/config.pyi | 23 +++++-- python/tests/test_config.py | 65 ++++++++++++++----- scripts/common-setup.sh | 2 + 4 files changed, 81 insertions(+), 23 deletions(-) diff --git a/monarch_hyperactor/src/config.rs b/monarch_hyperactor/src/config.rs index bea80671b..62cdb4aaa 100644 --- a/monarch_hyperactor/src/config.rs +++ b/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` 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__", diff --git a/python/monarch/_rust_bindings/monarch_hyperactor/config.pyi b/python/monarch/_rust_bindings/monarch_hyperactor/config.pyi index c9dba4c57..25bc42c9e 100644 --- a/python/monarch/_rust_bindings/monarch_hyperactor/config.pyi +++ b/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]: ... diff --git a/python/tests/test_config.py b/python/tests/test_config.py index deee1065c..4efc77a29 100644 --- a/python/tests/test_config.py +++ b/python/tests/test_config.py @@ -6,14 +6,33 @@ # 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, @@ -21,32 +40,42 @@ def test_get_set_transport() -> None: 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 diff --git a/scripts/common-setup.sh b/scripts/common-setup.sh index 2bfb5ae8b..272da3e53 100755 --- a/scripts/common-setup.sh +++ b/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..."