diff --git a/changelog.d/20241008_092953_kevin_improve_double_registration_error_message.rst b/changelog.d/20241008_092953_kevin_improve_double_registration_error_message.rst new file mode 100644 index 000000000..ad8d305b9 --- /dev/null +++ b/changelog.d/20241008_092953_kevin_improve_double_registration_error_message.rst @@ -0,0 +1,4 @@ +Bug Fixes +^^^^^^^^^ + +- Fix ``function_id`` in error message that previously referenced ``None`` diff --git a/compute_sdk/globus_compute_sdk/sdk/executor.py b/compute_sdk/globus_compute_sdk/sdk/executor.py index 7432bdab4..8bb66bffc 100644 --- a/compute_sdk/globus_compute_sdk/sdk/executor.py +++ b/compute_sdk/globus_compute_sdk/sdk/executor.py @@ -453,15 +453,24 @@ def register_function( the ``Client.register_function()``. :returns: the function's ``function_id`` string, as returned by registration upstream + :raises ValueError: raised if a function has already been registered with + this Executor """ if self._stopped: raise RuntimeError(f"{self!r} is shutdown; refusing to register function") fn_cache_key = self._fn_cache_key(fn) if fn_cache_key in self._function_registry: - msg = f"Function already registered as function id: {function_id}" + cached_fn_id = self._function_registry[fn_cache_key] + msg = f"Function already registered as function id: {cached_fn_id}" + if function_id: + if function_id == cached_fn_id: + log.warning(msg) + return cached_fn_id + + msg += f" (attempted id: {function_id})" + log.error(msg) - self.shutdown(wait=False, cancel_futures=True) raise ValueError(msg) if function_id: diff --git a/compute_sdk/tests/unit/test_executor.py b/compute_sdk/tests/unit/test_executor.py index 1da47e335..1656f7947 100644 --- a/compute_sdk/tests/unit/test_executor.py +++ b/compute_sdk/tests/unit/test_executor.py @@ -379,33 +379,48 @@ def some_func(*a, **k): gce._task_submitter.is_alive.return_value = False # allow test cleanup -def test_multiple_register_function_fails(gc_executor): +def test_multiple_register_function_fails(gc_executor, mock_log): gcc, gce = gc_executor gcc.register_function.return_value = "abc" - gce.register_function(noop) + exp_f_id = gce.register_function(noop) - with pytest.raises(ValueError): - gce.register_function(noop) + f_id = gce.register_function(noop, function_id=exp_f_id) + assert f_id == exp_f_id, "Explicitly same func_id re-registered allowed ..." + assert mock_log.warning.called, "... but is odd, so warn about it" - try_assert(lambda: gce._stopped) + a, k = mock_log.warning.call_args + assert "Function already registered" in a[0] + assert f_id in a[0] - with pytest.raises(RuntimeError): + with pytest.raises(ValueError) as pyt_e: gce.register_function(noop) + e_str = str(pyt_e.value) + assert "Function already registered" in e_str + assert f_id in e_str + assert "attempted" not in e_str + assert not gce._stopped + def test_shortcut_register_function(gc_executor): gcc, gce = gc_executor fn_id = str(uuid.uuid4()) + other_id = str(uuid.uuid4()) gce.register_function(noop, function_id=fn_id) - with pytest.raises(ValueError) as pyt_exc: - gce.register_function(noop, function_id=fn_id) + with pytest.raises(ValueError) as pyt_e: + gce.register_function(noop, function_id=other_id) - assert "Function already registered" in str(pyt_exc.value) - gcc.register_function.assert_not_called() + e_str = str(pyt_e.value) + assert "Function already registered" in e_str + assert fn_id in e_str + assert f"attempted id: {other_id}" in e_str + assert not gce._stopped + + assert not gcc.register_function.called def test_failed_registration_shuts_down_executor(gc_executor, randomstring): @@ -420,6 +435,13 @@ def test_failed_registration_shuts_down_executor(gc_executor, randomstring): assert pyt_exc.value is exc, "Expected raw exception raised" try_assert(lambda: gce._stopped) + with pytest.raises(RuntimeError) as pyt_e: + gce.register_function(noop) + + e_str = str(pyt_e.value) + assert "is shutdown" in e_str + assert "refusing to register function" in e_str + @pytest.mark.parametrize("container_id", (None, uuid.uuid4(), str(uuid.uuid4()))) def test_container_id_as_id(gc_executor, container_id):