Skip to content

Commit

Permalink
Unbreak windows tests (and likely the windows multiprocess executor i…
Browse files Browse the repository at this point in the history
…n general) (#11679)

Summary:
SIGKILL is not a thing on windows

### Summary & Motivation

### How I Tested These Changes
  • Loading branch information
gibsondan committed Jan 12, 2023
1 parent 909356b commit 43960fc
Show file tree
Hide file tree
Showing 4 changed files with 11 additions and 12 deletions.
9 changes: 1 addition & 8 deletions python_modules/dagster/dagster/_core/test_utils.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import asyncio
import os
import re
import signal
import sys
import tempfile
import time
Expand Down Expand Up @@ -36,7 +35,7 @@
from dagster._legacy import ModeDefinition, pipeline, solid
from dagster._serdes import ConfigurableClass
from dagster._seven.compat.pendulum import create_pendulum_time, mock_pendulum_timezone
from dagster._utils import Counter, traced, traced_counter
from dagster._utils import Counter, get_terminate_signal, traced, traced_counter
from dagster._utils.error import serializable_error_info_from_exc_info
from dagster._utils.log import configure_loggers
from dagster._utils.merger import merge_dicts
Expand Down Expand Up @@ -507,12 +506,6 @@ def from_config_value(inst_data, config_value):
return TestSecretsLoader(inst_data=inst_data, **config_value)


def get_terminate_signal():
if sys.platform == "win32":
return signal.SIGTERM
return signal.SIGKILL


def get_crash_signals():
return [get_terminate_signal()]

Expand Down
8 changes: 7 additions & 1 deletion python_modules/dagster/dagster/_utils/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -669,14 +669,20 @@ def inner(*args, **kwargs):
return cast(T_Callable, inner)


def get_terminate_signal():
if sys.platform == "win32":
return signal.SIGTERM
return signal.SIGKILL


def get_run_crash_explanation(prefix: str, exit_code: int):
# As per https://docs.python.org/3/library/subprocess.html#subprocess.CompletedProcess.returncode
# negative exit code means a posix signal
if exit_code < 0 and -exit_code in [signal.value for signal in Signals]:
posix_signal = -exit_code
signal_str = Signals(posix_signal).name
exit_clause = f"was terminated by signal {posix_signal} ({signal_str})."
if posix_signal == Signals.SIGKILL:
if posix_signal == get_terminate_signal():
exit_clause = (
exit_clause
+ " This usually indicates that the process was"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from signal import Signals

from dagster._utils import get_run_crash_explanation
from dagster._utils import get_run_crash_explanation, get_terminate_signal


def test_get_run_crash_explanation():
Expand All @@ -12,5 +12,5 @@ def test_get_run_crash_explanation():
assert (
"foo was terminated by signal 9 (SIGKILL). This usually indicates that the process was"
" killed by the operating system due to running out of memory."
in get_run_crash_explanation("foo", -Signals.SIGKILL)
in get_run_crash_explanation("foo", -get_terminate_signal())
)
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@
cleanup_test_instance,
create_test_daemon_workspace_context,
get_crash_signals,
get_terminate_signal,
)
from dagster._seven import IS_WINDOWS
from dagster._seven.compat.pendulum import create_pendulum_time, to_timezone
from dagster._utils import get_terminate_signal

from .conftest import workspace_load_target
from .test_scheduler_run import (
Expand Down

0 comments on commit 43960fc

Please sign in to comment.