From 3bbc14f26a9d0ae9abf8a4b8835f26480cd60cf8 Mon Sep 17 00:00:00 2001 From: Jason Boutte Date: Tue, 26 May 2026 15:46:29 -0700 Subject: [PATCH 1/5] fix: refactor data server protocols to be lazy --- CIME/Servers/__init__.py | 89 +++++++++++++++++++----- CIME/case/check_input_data.py | 4 +- CIME/tests/test_unit_servers.py | 117 ++++++++++++++++++++++++++++++++ 3 files changed, 191 insertions(+), 19 deletions(-) create mode 100644 CIME/tests/test_unit_servers.py diff --git a/CIME/Servers/__init__.py b/CIME/Servers/__init__.py index fb8307ba59d..637f106587c 100644 --- a/CIME/Servers/__init__.py +++ b/CIME/Servers/__init__.py @@ -1,19 +1,74 @@ -# pylint: disable=import-error +""" +CIME Server implementations for data transfer. + +Server availability is detected lazily on first access to avoid +running executables at import time. +""" + +from functools import lru_cache from shutil import which -has_gftp = which("globus-url-copy") -has_svn = which("svn") -has_wget = which("wget") -has_ftp = True -try: - from ftplib import FTP -except ImportError: - has_ftp = False -if has_ftp: - from CIME.Servers.ftp import FTP -if has_svn: - from CIME.Servers.svn import SVN -if has_wget: - from CIME.Servers.wget import WGET -if has_gftp: - from CIME.Servers.gftp import GridFTP + +@lru_cache(maxsize=None) +def is_protocol_available(protocol: str) -> bool: + """ + Check if a protocol is available. + + Args: + protocol: One of 'ftp', 'svn', 'wget', 'gftp' + + Returns: + True if the protocol is available, False otherwise. + """ + protocol = protocol.lower() + if protocol == "ftp": + try: + from ftplib import FTP # noqa: F401 pylint: disable=unused-import + + return True + except ImportError: + return False + elif protocol == "svn": + return which("svn") is not None + elif protocol == "wget": + return which("wget") is not None + elif protocol == "gftp": + return which("globus-url-copy") is not None + return False + + +def __getattr__(name: str): + """Lazy loading of server classes and has_* attributes.""" + if name == "FTP": + if is_protocol_available("ftp"): + from CIME.Servers.ftp import FTP + + return FTP + raise AttributeError("FTP server not available") + elif name == "SVN": + if is_protocol_available("svn"): + from CIME.Servers.svn import SVN + + return SVN + raise AttributeError("SVN server not available (svn not found)") + elif name == "WGET": + if is_protocol_available("wget"): + from CIME.Servers.wget import WGET + + return WGET + raise AttributeError("WGET server not available (wget not found)") + elif name == "GridFTP": + if is_protocol_available("gftp"): + from CIME.Servers.gftp import GridFTP + + return GridFTP + raise AttributeError("GridFTP server not available (globus-url-copy not found)") + elif name == "has_ftp": + return is_protocol_available("ftp") + elif name == "has_svn": + return is_protocol_available("svn") + elif name == "has_wget": + return is_protocol_available("wget") + elif name == "has_gftp": + return is_protocol_available("gftp") + raise AttributeError(f"module {__name__!r} has no attribute {name!r}") diff --git a/CIME/case/check_input_data.py b/CIME/case/check_input_data.py index cc67e1c6aca..8a312ac5528 100644 --- a/CIME/case/check_input_data.py +++ b/CIME/case/check_input_data.py @@ -24,7 +24,7 @@ def _download_checksum_file(rundir): # download and merge all available chksum files. while protocol is not None: protocol, address, user, passwd, chksum_file, _, _ = inputdata.get_next_server() - if protocol not in vars(CIME.Servers): + if not CIME.Servers.is_protocol_available(protocol): logger.info("Client protocol {} not enabled".format(protocol)) continue logger.info( @@ -414,7 +414,7 @@ def _check_input_data_impl( no_files_missing = True server = None if download: - if protocol not in vars(CIME.Servers): + if not CIME.Servers.is_protocol_available(protocol): logger.info("Client protocol {} not enabled".format(protocol)) return False logger.info( diff --git a/CIME/tests/test_unit_servers.py b/CIME/tests/test_unit_servers.py new file mode 100644 index 00000000000..cd8df59b139 --- /dev/null +++ b/CIME/tests/test_unit_servers.py @@ -0,0 +1,117 @@ +"""Unit tests for CIME.Servers lazy loading. + +These tests verify: +1. Lazy loading behavior (no subprocess at import time) +2. Backward compatibility with existing usage patterns +3. is_protocol_available() API +""" + +import pytest + + +class TestServersLazyLoading: + """Tests for lazy loading of server modules.""" + + def test_import_does_not_run_which(self): + """Importing CIME.Servers should not run shutil.which().""" + import CIME.Servers + + assert CIME.Servers is not None + + def test_availability_not_checked_at_import(self): + """Availability flags should be unchecked after import.""" + import importlib + import sys + + # Force reimport + for mod in list(sys.modules.keys()): + if "CIME.Servers" in mod: + del sys.modules[mod] + + import CIME.Servers + + assert CIME.Servers is not None + + +class TestProtocolAvailability: + """Tests for is_protocol_available() API.""" + + def test_is_protocol_available_ftp(self): + """FTP should always be available (stdlib).""" + import CIME.Servers + + assert CIME.Servers.is_protocol_available("ftp") is True + + def test_is_protocol_available_invalid(self): + """Invalid protocol should return False.""" + import CIME.Servers + + assert CIME.Servers.is_protocol_available("invalid") is False + + def test_is_protocol_available_case_insensitive(self): + """Protocol check should be case-insensitive.""" + import CIME.Servers + + assert CIME.Servers.is_protocol_available("FTP") is True + assert CIME.Servers.is_protocol_available("Ftp") is True + assert CIME.Servers.is_protocol_available("ftp") is True + + +class TestServerClassAccess: + """Tests for accessing server classes.""" + + def test_ftp_attribute_access(self): + """Accessing CIME.Servers.FTP should work via __getattr__.""" + import CIME.Servers + + FTP = CIME.Servers.FTP + assert FTP is not None + assert FTP.__name__ == "FTP" + + def test_ftp_has_ftp_login_method(self): + """FTP class should have ftp_login class method (used by check_input_data).""" + import CIME.Servers + + assert hasattr(CIME.Servers.FTP, "ftp_login") + + def test_wget_has_wget_login_method(self): + """WGET class should have wget_login class method if available.""" + import CIME.Servers + + if CIME.Servers.is_protocol_available("wget"): + assert hasattr(CIME.Servers.WGET, "wget_login") + + def test_unavailable_server_raises_attribute_error(self): + """Accessing unavailable server should raise AttributeError.""" + import CIME.Servers + + if not CIME.Servers.is_protocol_available("gftp"): + with pytest.raises(AttributeError): + _ = CIME.Servers.GridFTP + + +class TestBackwardCompatibility: + """Tests for backward compatibility with existing code patterns.""" + + def test_has_ftp_attribute(self): + """has_ftp attribute should return True (FTP always available).""" + import CIME.Servers + + assert CIME.Servers.has_ftp is True + + def test_has_attributes_exist(self): + """All has_* attributes should be accessible.""" + import CIME.Servers + + # These should not raise, regardless of availability + _ = CIME.Servers.has_ftp + _ = CIME.Servers.has_svn + _ = CIME.Servers.has_wget + _ = CIME.Servers.has_gftp + + def test_instantiate_ftp_server(self): + """Should be able to instantiate FTP server (backward compat).""" + import CIME.Servers + + FTP = CIME.Servers.FTP + assert FTP is not None From 70553fea3a3d6afb2bab7d67a3143ab76c6f8466 Mon Sep 17 00:00:00 2001 From: Jason Boutte Date: Tue, 21 Jul 2026 14:44:17 -0700 Subject: [PATCH 2/5] Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- CIME/tests/test_unit_servers.py | 40 ++++++++++++++++++++++++--------- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/CIME/tests/test_unit_servers.py b/CIME/tests/test_unit_servers.py index cd8df59b139..2e135db6f8b 100644 --- a/CIME/tests/test_unit_servers.py +++ b/CIME/tests/test_unit_servers.py @@ -1,7 +1,7 @@ """Unit tests for CIME.Servers lazy loading. These tests verify: -1. Lazy loading behavior (no subprocess at import time) +1. Lazy loading behavior (no shutil.which() / executable availability checks at import time) 2. Backward compatibility with existing usage patterns 3. is_protocol_available() API """ @@ -12,26 +12,44 @@ class TestServersLazyLoading: """Tests for lazy loading of server modules.""" - def test_import_does_not_run_which(self): + def test_import_does_not_run_which(self, monkeypatch): """Importing CIME.Servers should not run shutil.which().""" - import CIME.Servers - - assert CIME.Servers is not None - - def test_availability_not_checked_at_import(self): - """Availability flags should be unchecked after import.""" import importlib + import shutil import sys + def _boom(*_args, **_kwargs): + raise AssertionError("shutil.which() should not be called at import time") + + monkeypatch.setattr(shutil, "which", _boom) + # Force reimport - for mod in list(sys.modules.keys()): - if "CIME.Servers" in mod: + for mod in list(sys.modules): + if mod == "CIME.Servers" or mod.startswith("CIME.Servers."): del sys.modules[mod] + importlib.import_module("CIME.Servers") + + def test_availability_checked_lazily_and_cached(self, monkeypatch): + """Availability checks should run on first access and then be cached.""" import CIME.Servers - assert CIME.Servers is not None + calls = {"n": 0} + def _fake_which(_cmd): + calls["n"] += 1 + return None + + monkeypatch.setattr(CIME.Servers, "which", _fake_which) + CIME.Servers.is_protocol_available.cache_clear() + + assert calls["n"] == 0 # no availability checks at import time + + _ = CIME.Servers.has_svn + assert calls["n"] == 1 + + _ = CIME.Servers.has_svn + assert calls["n"] == 1 class TestProtocolAvailability: """Tests for is_protocol_available() API.""" From 735760d2bfa1a999a23fab4640fcd24ebe16c05b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 21 Jul 2026 21:45:15 +0000 Subject: [PATCH 3/5] fix: guard None protocol before availability checks --- CIME/case/check_input_data.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CIME/case/check_input_data.py b/CIME/case/check_input_data.py index 8a312ac5528..622dc053b18 100644 --- a/CIME/case/check_input_data.py +++ b/CIME/case/check_input_data.py @@ -24,6 +24,8 @@ def _download_checksum_file(rundir): # download and merge all available chksum files. while protocol is not None: protocol, address, user, passwd, chksum_file, _, _ = inputdata.get_next_server() + if protocol is None: + continue if not CIME.Servers.is_protocol_available(protocol): logger.info("Client protocol {} not enabled".format(protocol)) continue @@ -414,6 +416,8 @@ def _check_input_data_impl( no_files_missing = True server = None if download: + if protocol is None: + return False if not CIME.Servers.is_protocol_available(protocol): logger.info("Client protocol {} not enabled".format(protocol)) return False From aba8461278b681bc047de453638344a445c116f8 Mon Sep 17 00:00:00 2001 From: Jason Boutte Date: Wed, 22 Jul 2026 00:13:49 -0700 Subject: [PATCH 4/5] fix: black formatting --- CIME/tests/test_unit_servers.py | 1 + 1 file changed, 1 insertion(+) diff --git a/CIME/tests/test_unit_servers.py b/CIME/tests/test_unit_servers.py index 2e135db6f8b..e9594ba693b 100644 --- a/CIME/tests/test_unit_servers.py +++ b/CIME/tests/test_unit_servers.py @@ -51,6 +51,7 @@ def _fake_which(_cmd): _ = CIME.Servers.has_svn assert calls["n"] == 1 + class TestProtocolAvailability: """Tests for is_protocol_available() API.""" From ab4c15faa00df22d33d3622ebd35b4897ab0b7e4 Mon Sep 17 00:00:00 2001 From: Jason Boutte Date: Wed, 22 Jul 2026 08:05:30 -0700 Subject: [PATCH 5/5] fix: resolve shutil.which dynamically to avoid import-time state leak Use 'import shutil' with runtime shutil.which() lookups instead of binding 'which' at import time, so force-reimporting CIME.Servers while shutil.which is patched can no longer permanently poison the module. Update the lazy-cache test to patch shutil.which directly. --- CIME/Servers/__init__.py | 8 ++++---- CIME/tests/test_unit_servers.py | 4 +++- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/CIME/Servers/__init__.py b/CIME/Servers/__init__.py index 637f106587c..d7b95033494 100644 --- a/CIME/Servers/__init__.py +++ b/CIME/Servers/__init__.py @@ -5,8 +5,8 @@ running executables at import time. """ +import shutil from functools import lru_cache -from shutil import which @lru_cache(maxsize=None) @@ -29,11 +29,11 @@ def is_protocol_available(protocol: str) -> bool: except ImportError: return False elif protocol == "svn": - return which("svn") is not None + return shutil.which("svn") is not None elif protocol == "wget": - return which("wget") is not None + return shutil.which("wget") is not None elif protocol == "gftp": - return which("globus-url-copy") is not None + return shutil.which("globus-url-copy") is not None return False diff --git a/CIME/tests/test_unit_servers.py b/CIME/tests/test_unit_servers.py index e9594ba693b..296dfd59b0b 100644 --- a/CIME/tests/test_unit_servers.py +++ b/CIME/tests/test_unit_servers.py @@ -32,6 +32,8 @@ def _boom(*_args, **_kwargs): def test_availability_checked_lazily_and_cached(self, monkeypatch): """Availability checks should run on first access and then be cached.""" + import shutil + import CIME.Servers calls = {"n": 0} @@ -40,7 +42,7 @@ def _fake_which(_cmd): calls["n"] += 1 return None - monkeypatch.setattr(CIME.Servers, "which", _fake_which) + monkeypatch.setattr(shutil, "which", _fake_which) CIME.Servers.is_protocol_available.cache_clear() assert calls["n"] == 0 # no availability checks at import time