Skip to content

Commit e270d1e

Browse files
author
Mohamed Zeidan
committed
fix: forward __delattr__ on the lazy JumpStart session proxy
_LazyJumpStartSagemakerSession declares __slots__ = () and forwards __getattr__/__setattr__ but not __delattr__. unittest.mock.patch reads the original via target.__dict__[name], which the proxy forwards to the real session's instance __dict__; for a class-level attribute such as Session.read_s3_file that raises KeyError, so mock records is_local=False and restores the attribute by calling delattr on teardown. Without __delattr__ that teardown raises AttributeError: '_LazyJumpStartSagemakerSession' object has no attribute ... and, worse, leaves the mock installed on the process-wide session, so unrelated tests in the same worker then see the mock instead of the real attribute. Forwarding __delattr__ removes the shadowing instance attribute created by patch's setattr, which makes the class-level attribute visible again. __slots__ is kept: dropping it does not fix this, because target.__dict__ would then resolve to the proxy's own empty dict and mock would still take the delattr branch. Adds two regression tests covering the class-level path (the defect) and the instance-level path (is_local=True, restored via setattr).
1 parent a28524a commit e270d1e

2 files changed

Lines changed: 55 additions & 0 deletions

File tree

‎sagemaker-core/src/sagemaker/core/jumpstart/constants.py‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,15 @@ def __getattr__(self, name):
240240
def __setattr__(self, name, value):
241241
setattr(type(self)._resolve(), name, value)
242242

243+
def __delattr__(self, name):
244+
# Forwarded so that ``unittest.mock.patch`` tears down cleanly. For a
245+
# class-level attribute mock records ``is_local=False``, sets the mock
246+
# via ``__setattr__`` (creating an instance attribute on the real
247+
# session) and restores by calling ``delattr``. Without this method the
248+
# teardown raises ``AttributeError`` and leaves the mock installed on
249+
# the process-wide session.
250+
delattr(type(self)._resolve(), name)
251+
243252
def __copy__(self):
244253
return copy.copy(type(self)._resolve())
245254

‎sagemaker-core/tests/unit/jumpstart/test_constants.py‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,3 +90,49 @@ def test_failed_build_degrades_to_none_contract(caplog):
9090
assert _LazyJumpStartSagemakerSession._resolve() is None
9191
with pytest.raises(AttributeError):
9292
_ = constants.DEFAULT_JUMPSTART_SAGEMAKER_SESSION.boto_region_name
93+
94+
95+
def test_mock_patch_of_class_level_attribute_tears_down_cleanly():
96+
"""``mock.patch`` on a class-level session method must restore the original and
97+
leave nothing behind on the process-wide session.
98+
99+
mock records ``is_local=False`` for a class-level attribute (it is absent from
100+
the instance ``__dict__``) and restores it by calling ``delattr``, so the proxy
101+
must forward ``__delattr__``. Without that forwarding the teardown raises
102+
``AttributeError`` and the mock leaks into every later test in the same worker.
103+
"""
104+
105+
class FakeSession:
106+
"""Stands in for ``Session``: ``read_s3_file`` is a class-level attribute."""
107+
108+
def read_s3_file(self):
109+
return "real"
110+
111+
fake = FakeSession()
112+
with patch.object(constants, "Session", return_value=fake):
113+
proxy = constants.DEFAULT_JUMPSTART_SAGEMAKER_SESSION
114+
assert proxy.read_s3_file() == "real"
115+
116+
with patch.object(proxy, "read_s3_file", return_value="mocked"):
117+
assert proxy.read_s3_file() == "mocked"
118+
119+
# Teardown must restore the class method and leave no shadowing instance
120+
# attribute behind on the shared session.
121+
assert proxy.read_s3_file() == "real"
122+
assert "read_s3_file" not in fake.__dict__
123+
124+
125+
def test_mock_patch_of_instance_level_attribute_tears_down_cleanly():
126+
"""The instance-attribute path (``is_local=True``, restored via ``setattr``)
127+
must keep working -- guards against a regression in ``__setattr__`` forwarding."""
128+
129+
class FakeSession:
130+
def __init__(self):
131+
self.sagemaker_client = "real-client"
132+
133+
fake = FakeSession()
134+
with patch.object(constants, "Session", return_value=fake):
135+
proxy = constants.DEFAULT_JUMPSTART_SAGEMAKER_SESSION
136+
with patch.object(proxy, "sagemaker_client", "mock-client"):
137+
assert proxy.sagemaker_client == "mock-client"
138+
assert proxy.sagemaker_client == "real-client"

0 commit comments

Comments
 (0)