Skip to content

Commit d16e4ed

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 9be072b commit d16e4ed

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
@@ -84,3 +84,49 @@ def test_failed_build_degrades_to_none_contract():
8484
assert _LazyJumpStartSagemakerSession._resolve() is None
8585
with pytest.raises(AttributeError):
8686
_ = constants.DEFAULT_JUMPSTART_SAGEMAKER_SESSION.boto_region_name
87+
88+
89+
def test_mock_patch_of_class_level_attribute_tears_down_cleanly():
90+
"""``mock.patch`` on a class-level session method must restore the original and
91+
leave nothing behind on the process-wide session.
92+
93+
mock records ``is_local=False`` for a class-level attribute (it is absent from
94+
the instance ``__dict__``) and restores it by calling ``delattr``, so the proxy
95+
must forward ``__delattr__``. Without that forwarding the teardown raises
96+
``AttributeError`` and the mock leaks into every later test in the same worker.
97+
"""
98+
99+
class FakeSession:
100+
"""Stands in for ``Session``: ``read_s3_file`` is a class-level attribute."""
101+
102+
def read_s3_file(self):
103+
return "real"
104+
105+
fake = FakeSession()
106+
with patch.object(constants, "Session", return_value=fake):
107+
proxy = constants.DEFAULT_JUMPSTART_SAGEMAKER_SESSION
108+
assert proxy.read_s3_file() == "real"
109+
110+
with patch.object(proxy, "read_s3_file", return_value="mocked"):
111+
assert proxy.read_s3_file() == "mocked"
112+
113+
# Teardown must restore the class method and leave no shadowing instance
114+
# attribute behind on the shared session.
115+
assert proxy.read_s3_file() == "real"
116+
assert "read_s3_file" not in fake.__dict__
117+
118+
119+
def test_mock_patch_of_instance_level_attribute_tears_down_cleanly():
120+
"""The instance-attribute path (``is_local=True``, restored via ``setattr``)
121+
must keep working -- guards against a regression in ``__setattr__`` forwarding."""
122+
123+
class FakeSession:
124+
def __init__(self):
125+
self.sagemaker_client = "real-client"
126+
127+
fake = FakeSession()
128+
with patch.object(constants, "Session", return_value=fake):
129+
proxy = constants.DEFAULT_JUMPSTART_SAGEMAKER_SESSION
130+
with patch.object(proxy, "sagemaker_client", "mock-client"):
131+
assert proxy.sagemaker_client == "mock-client"
132+
assert proxy.sagemaker_client == "real-client"

0 commit comments

Comments
 (0)