Skip to content

Commit bf0f5f5

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__. 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 leaves the mock installed on the process-wide session, so unrelated tests in the same xdist worker then see the mock instead of the real attribute. That is what made test_notebook_utils, test_model, test_sagemaker_config and test_js_builder fail on py39-py312. 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 f1591b5 commit bf0f5f5

2 files changed

Lines changed: 55 additions & 0 deletions

File tree

‎src/sagemaker/jumpstart/constants.py‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,15 @@ def __getattr__(self, name):
236236
def __setattr__(self, name, value):
237237
setattr(type(self)._resolve(), name, value)
238238

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

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

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

0 commit comments

Comments
 (0)