Skip to content
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

fix: Bypass signal handler if running in a thread #3251

Merged
merged 6 commits into from
May 21, 2024
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 22 additions & 3 deletions packages/phoenix-evals/src/phoenix/evals/executors.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import asyncio
import logging
import signal
import threading
import traceback
from typing import Any, Callable, Coroutine, List, Optional, Protocol, Sequence, Tuple, Union

Expand Down Expand Up @@ -244,7 +245,7 @@ def __init__(
max_retries: int = 10,
exit_on_error: bool = True,
fallback_return_value: Union[Unset, Any] = _unset,
termination_signal: signal.Signals = signal.SIGINT,
termination_signal: Optional[signal.Signals] = signal.SIGINT,
):
self.generate = generation_fn
self.fallback_return_value = fallback_return_value
Expand All @@ -259,8 +260,18 @@ def _signal_handler(self, signum: int, frame: Any) -> None:
tqdm.write("Process was interrupted. The return value will be incomplete...")
self._TERMINATE = True

def _set_signal_handler(
self, signum: Optional[int], handler: Callable[[int, Any], None]
) -> None:
if signum is not None:
signal.signal(signum, handler)

def _reset_signal_handler(self, signum: Optional[int]) -> None:
if signum is not None:
signal.signal(signum, signal.SIG_DFL)

def run(self, inputs: Sequence[Any]) -> List[Any]:
signal.signal(self.termination_signal, self._signal_handler)
self._set_signal_handler(self.termination_signal, self._signal_handler)
outputs = [self.fallback_return_value] * len(inputs)
progress_bar = tqdm(total=len(inputs), bar_format=self.tqdm_bar_format)

Expand All @@ -287,7 +298,7 @@ def run(self, inputs: Sequence[Any]) -> List[Any]:
return outputs
else:
progress_bar.update()
signal.signal(self.termination_signal, signal.SIG_DFL) # reset the SIGTERM handler
self._reset_signal_handler(self.termination_signal)
return outputs


Expand All @@ -300,12 +311,19 @@ def get_executor_on_sync_context(
exit_on_error: bool = True,
fallback_return_value: Union[Unset, Any] = _unset,
) -> Executor:
if threading.current_thread() is threading.main_thread():
# only handle termination signals in the main thread
termination_signal = None
else:
termination_signal = signal.SIGINT

if run_sync:
return SyncExecutor(
sync_fn,
tqdm_bar_format=tqdm_bar_format,
exit_on_error=exit_on_error,
fallback_return_value=fallback_return_value,
termination_signal=termination_signal,
)

if _running_event_loop_exists():
Expand All @@ -328,6 +346,7 @@ def get_executor_on_sync_context(
tqdm_bar_format=tqdm_bar_format,
exit_on_error=exit_on_error,
fallback_return_value=fallback_return_value,
termination_signal=termination_signal,
)
else:
return AsyncExecutor(
Expand Down
Loading