-
Notifications
You must be signed in to change notification settings - Fork 224
Fixes lazy load server protocols #4984
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+218
−21
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
3bbc14f
fix: refactor data server protocols to be lazy
jasonb5 70553fe
Apply suggestions from code review
jasonb5 735760d
fix: guard None protocol before availability checks
Copilot aba8461
fix: black formatting
jasonb5 ab4c15f
fix: resolve shutil.which dynamically to avoid import-time state leak
jasonb5 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,19 +1,74 @@ | ||
| # pylint: disable=import-error | ||
| 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 | ||
| """ | ||
| CIME Server implementations for data transfer. | ||
|
|
||
| Server availability is detected lazily on first access to avoid | ||
| running executables at import time. | ||
| """ | ||
|
|
||
| import shutil | ||
| from functools import lru_cache | ||
|
|
||
|
|
||
| @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 shutil.which("svn") is not None | ||
| elif protocol == "wget": | ||
| return shutil.which("wget") is not None | ||
| elif protocol == "gftp": | ||
| return shutil.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}") | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,138 @@ | ||
| """Unit tests for CIME.Servers lazy loading. | ||
|
|
||
| These tests verify: | ||
| 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 | ||
|
jasonb5 marked this conversation as resolved.
|
||
| """ | ||
|
|
||
| import pytest | ||
|
|
||
|
|
||
| class TestServersLazyLoading: | ||
| """Tests for lazy loading of server modules.""" | ||
|
|
||
| def test_import_does_not_run_which(self, monkeypatch): | ||
| """Importing CIME.Servers should not run shutil.which().""" | ||
| 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): | ||
| 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 shutil | ||
|
|
||
| import CIME.Servers | ||
|
|
||
| calls = {"n": 0} | ||
|
|
||
| def _fake_which(_cmd): | ||
| calls["n"] += 1 | ||
| return None | ||
|
|
||
| monkeypatch.setattr(shutil, "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.""" | ||
|
|
||
| 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 | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot The typing explicitly requires protocol to have a value, let's guard at the call site against a None value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 735760d: I added call-site guards so we no longer call
CIME.Servers.is_protocol_available(...)whenprotocolisNone(in both_download_checksum_fileand_check_input_data_impl).