Skip to content

Commit

Permalink
Reduce spurious DNS lookups in tests
Browse files Browse the repository at this point in the history
The core of this change revolves around:

- reducing splatter of tests that don't need the `GlobusComputeEngine`; these
  mostly use `ThreadPoolEngine` now

- Specifying `address="127.0.0.1"` for those tests that do need the
  `GlobusComputeEngine` -- for the tests, there should be no need to look
  farther than home

At a cleanliness level, this set of changes should not be controversial -- the
tests have no business making arbitrary lookups of the network.  Pragmatically,
this also improves performance (negligible on fast networks, but by a good
amount on slow networks).
  • Loading branch information
khk-globus committed Nov 4, 2024
1 parent 2684bf9 commit 8b14b16
Show file tree
Hide file tree
Showing 7 changed files with 60 additions and 31 deletions.
3 changes: 2 additions & 1 deletion compute_endpoint/globus_compute_endpoint/engines/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,8 @@ def run_in_loop(self, target: t.Callable, *args) -> None:

def stop(self) -> None:
self._shutdown_event.set()
self._thread.join(timeout=0.1)
if self._thread.is_alive():
self._thread.join(timeout=0.1)


class GlobusComputeEngineBase(ABC):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -504,5 +504,6 @@ def executor_exception(self) -> t.Optional[Exception]:

def shutdown(self, /, **kwargs) -> None:
self._status_report_thread.stop()
self.job_status_poller.close()
if hasattr(self, "job_status_poller"):
self.job_status_poller.close()
self.executor.shutdown()
1 change: 1 addition & 0 deletions compute_endpoint/tests/unit/test_boot_persistence.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ def fake_ep_dir(fs: fakefs.FakeFilesystem, ep_name) -> pathlib.Path:
display_name: null
engine:
type: GlobusComputeEngine
address: 127.0.0.1
provider:
type: LocalProvider
init_blocks: 1
Expand Down
17 changes: 11 additions & 6 deletions compute_endpoint/tests/unit/test_cli_behavior.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
from globus_compute_endpoint.endpoint.config import Config
from globus_compute_endpoint.endpoint.config.utils import load_config_yaml
from globus_compute_endpoint.endpoint.endpoint import Endpoint
from globus_compute_endpoint.engines import ThreadPoolEngine
from globus_compute_sdk.sdk.auth.auth_client import ComputeAuthClient
from globus_compute_sdk.sdk.auth.globus_app import UserApp
from globus_compute_sdk.sdk.compute_dir import ensure_compute_dir
Expand Down Expand Up @@ -103,6 +104,7 @@ def func(name=ep_name, ep_uuid=None):
display_name: null
engine:
type: GlobusComputeEngine
address: 127.0.0.1
provider:
type: LocalProvider
init_blocks: 1
Expand All @@ -115,6 +117,7 @@ def func(name=ep_name, ep_uuid=None):
heartbeat_period: {{ heartbeat }}
engine:
type: GlobusComputeEngine
address: 127.0.0.1
provider:
type: LocalProvider
init_blocks: 1
Expand Down Expand Up @@ -278,7 +281,7 @@ def test_start_ep_reads_stdin(
):
data_is_valid, data = stdin_data

conf = Config()
conf = Config(executors=[ThreadPoolEngine])
mock_load_conf = mocker.patch(f"{_MOCK_BASE}load_config_yaml")
mock_load_conf.return_value = conf
mock_get_config = mocker.patch(f"{_MOCK_BASE}get_config")
Expand Down Expand Up @@ -466,10 +469,11 @@ def test_config_yaml_display_none(
config_cmd += f" --display-name {display_name}"
run_line(config_cmd)

with open(conf) as f:
conf_dict = load_config_yaml(f)
conf_dict = dict(yaml.safe_load(conf.read_text()))
conf_dict["engine"]["address"] = "127.0.0.1" # avoid unnecessary DNS lookup
conf = load_config_yaml(yaml.safe_dump(conf_dict))

assert conf_dict.display_name is None
assert conf.display_name is None, conf.display_name


def test_start_ep_incorrect_config_yaml(
Expand Down Expand Up @@ -525,7 +529,8 @@ def test_start_ep_config_py_takes_precedence(
mock_ep, *_ = mock_cli_state
conf_py.write_text(
"from globus_compute_endpoint.endpoint.config import Config"
"\nconfig = Config()"
"\nfrom globus_compute_endpoint.engines import ThreadPoolEngine"
"\nconfig = Config(executors=[ThreadPoolEngine()])"
)

run_line(f"start {ep_name}")
Expand Down Expand Up @@ -630,7 +635,7 @@ def test_die_with_parent_detached(
ep_name,
make_endpoint_dir,
):
config = Config()
config = Config(executors=[ThreadPoolEngine()])
mock_get_config.return_value = config
make_endpoint_dir()

Expand Down
7 changes: 5 additions & 2 deletions compute_endpoint/tests/unit/test_endpoint_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

@pytest.fixture
def config_dict():
return {"engine": {"type": "GlobusComputeEngine"}}
return {"engine": {"type": "GlobusComputeEngine", "address": "127.0.0.1"}}


@pytest.fixture
Expand All @@ -18,6 +18,7 @@ def config_dict_mu(tmp_path):
return {
"identity_mapping_config_path": idc,
"multi_user": True,
"executors": [], # remove unnecessary DNS calls; given historical design
}


Expand Down Expand Up @@ -95,7 +96,7 @@ def test_config_warns_bad_identity_mapping_path(mocker, config_dict_mu, tmp_path

@pytest.mark.parametrize("public", (None, True, False, "a", 1))
def test_public(public: t.Any):
c = Config(public=public)
c = Config(public=public, executors=[])
assert c.public is (public is True)


Expand All @@ -106,6 +107,7 @@ def test_conditional_engine_strategy(
):
config_dict["engine"]["type"] = engine_type
config_dict["engine"]["strategy"] = strategy
config_dict["engine"]["address"] = "127.0.0.1"

if engine_type == "GlobusComputeEngine":
if isinstance(strategy, str) or strategy is None:
Expand Down Expand Up @@ -138,6 +140,7 @@ def test_provider_container_compatibility(
):
config_dict["engine"]["container_uri"] = "docker://ubuntu"
config_dict["engine"]["provider"] = {"type": provider_type}
config_dict["engine"]["address"] = "127.0.0.1"

if compatible:
ConfigModel(**config_dict)
Expand Down
40 changes: 29 additions & 11 deletions compute_endpoint/tests/unit/test_endpoint_unit.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@
serialize_config,
)
from globus_compute_endpoint.endpoint.endpoint import Endpoint
from globus_compute_endpoint.engines import (
GlobusComputeEngine,
ProcessPoolEngine,
ThreadPoolEngine,
)
from globus_sdk import GlobusAPIError, NetworkError
from pytest_mock import MockFixture

Expand Down Expand Up @@ -105,14 +110,18 @@ def create_response(endpoint_id=endpoint_uuid, status_code=200, msg="Error Msg")


@pytest.fixture
def mock_ep_data(fs):
def conf():
yield Config(executors=[ThreadPoolEngine])


@pytest.fixture
def mock_ep_data(fs, conf):
ep = endpoint.Endpoint()
ep_dir = pathlib.Path("/some/path/mock_endpoint")
ep_dir.mkdir(parents=True, exist_ok=True)
log_to_console = False
no_color = True
ep_conf = Config()
yield ep, ep_dir, log_to_console, no_color, ep_conf
yield ep, ep_dir, log_to_console, no_color, conf


@pytest.fixture
Expand Down Expand Up @@ -498,7 +507,10 @@ def test_pid_file_check(pid_info, fs):
assert should_active == pid_status["active"]


def test_endpoint_get_metadata(mocker):
@pytest.mark.parametrize(
"engine_cls", (GlobusComputeEngine, ThreadPoolEngine, ProcessPoolEngine)
)
def test_endpoint_get_metadata(mocker, engine_cls):
mock_data = {
"endpoint_version": "106.7",
"hostname": "oneohtrix.never",
Expand All @@ -516,18 +528,26 @@ def test_endpoint_get_metadata(mocker):
mock_pwuid = mocker.patch("globus_compute_endpoint.endpoint.endpoint.pwd.getpwuid")
mock_pwuid.return_value = SimpleNamespace(pw_name=mock_data["local_user"])

test_config = Config()
k = {}
if engine_cls is GlobusComputeEngine:
k["address"] = "127.0.0.1"
executors = [engine_cls(**k)]
test_config = Config(executors=executors)
test_config.source_content = "foo: bar"
meta = Endpoint.get_metadata(test_config)

for e in test_config.executors:
e.shutdown()

for k, v in mock_data.items():
assert meta[k] == v

assert meta["endpoint_config"] == test_config.source_content
config = meta["config"]
assert len(config["executors"]) == 1
assert config["executors"][0]["type"] == "GlobusComputeEngine"
assert config["executors"][0]["executor"]["provider"]["type"] == "LocalProvider"
assert config["executors"][0]["type"] == engine_cls.__name__
if engine_cls is GlobusComputeEngine:
assert config["executors"][0]["executor"]["provider"]["type"] == "LocalProvider"


@pytest.mark.parametrize("env", [None, "blar", "local", "production"])
Expand Down Expand Up @@ -698,7 +718,7 @@ def test_always_prints_endpoint_id_to_terminal(mocker, mock_ep_data, mock_reg_in


def test_serialize_config_field_types():
ep_config = Config()
ep_config = Config(executors=[GlobusComputeEngine(address="127.0.0.1")])

ep_config._hidden_attr = "123"
ep_config.rando_attr = "howdy"
Expand Down Expand Up @@ -896,9 +916,7 @@ def test_delete_endpoint_with_uuid_happy(
mocker: MockFixture,
mock_ep_data: tuple[Endpoint, pathlib.Path, bool, bool, Config],
):
ep = mock_ep_data[0]
ep_dir = mock_ep_data[1]
ep_config = mock_ep_data[4]
ep, ep_dir, *_, ep_config = mock_ep_data
ep_uuid = str(uuid.uuid4())
mock_log = mocker.patch(f"{_mock_base}log")
mock_gcc = mocker.Mock()
Expand Down
20 changes: 10 additions & 10 deletions compute_endpoint/tests/unit/test_engines.py
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ def test_serialized_engine_config_has_provider(


def test_gcengine_compute_launch_cmd():
engine = GlobusComputeEngine()
engine = GlobusComputeEngine(address="127.0.0.1")
assert engine.executor.launch_cmd.startswith(
"globus-compute-endpoint python-exec"
" parsl.executors.high_throughput.process_worker_pool"
Expand All @@ -164,7 +164,7 @@ def test_gcengine_compute_launch_cmd():


def test_gcengine_compute_interchange_launch_cmd():
engine = GlobusComputeEngine()
engine = GlobusComputeEngine(address="127.0.0.1")
assert engine.executor.interchange_launch_cmd[:3] == [
"globus-compute-endpoint",
"python-exec",
Expand Down Expand Up @@ -250,20 +250,20 @@ def test_gcengine_encrypted(encrypted: bool, engine_runner):


def test_gcengine_new_executor_not_exceptional():
gce = GlobusComputeEngine()
gce = GlobusComputeEngine(address="127.0.0.1")
assert gce.executor_exception is None, "Expect no exception from fresh Executor"


def test_gcengine_executor_exception_passthrough(randomstring):
gce = GlobusComputeEngine()
gce = GlobusComputeEngine(address="127.0.0.1")
exc_text = randomstring()
gce.executor.set_bad_state_and_fail_all(ZeroDivisionError(exc_text))
assert isinstance(gce.executor_exception, ZeroDivisionError)
assert exc_text in str(gce.executor_exception)


def test_gcengine_bad_state_futures_failed_immediately(randomstring, task_uuid):
gce = GlobusComputeEngine()
gce = GlobusComputeEngine(address="127.0.0.1")
exc_text = randomstring()
gce.executor.set_bad_state_and_fail_all(ZeroDivisionError(exc_text))

Expand All @@ -278,7 +278,7 @@ def test_gcengine_bad_state_futures_failed_immediately(randomstring, task_uuid):


def test_gcengine_exception_report_from_bad_state(task_uuid):
gce = GlobusComputeEngine()
gce = GlobusComputeEngine(address="127.0.0.1")
gce.executor.set_bad_state_and_fail_all(ZeroDivisionError())

gce.submit(
Expand All @@ -300,19 +300,19 @@ def test_gcengine_exception_report_from_bad_state(task_uuid):

def test_gcengine_rejects_mpi_mode(randomstring):
with pytest.raises(ValueError) as pyt_exc_1:
GlobusComputeEngine(enable_mpi_mode=True)
GlobusComputeEngine(enable_mpi_mode=True, address="127.0.0.1")

assert "is not supported" in str(pyt_exc_1)

with pytest.raises(ValueError) as pyt_exc_2:
GlobusComputeEngine(mpi_launcher=randomstring())
GlobusComputeEngine(mpi_launcher=randomstring(), address="127.0.0.1")

assert "is not supported" in str(pyt_exc_2)


def test_gcengine_rejects_resource_specification(task_uuid):
with pytest.raises(ValueError) as pyt_exc:
GlobusComputeEngine().submit(
GlobusComputeEngine(address="127.0.0.1").submit(
str(task_uuid),
packed_task=b"packed_task",
resource_specification={"foo": "bar"},
Expand Down Expand Up @@ -342,7 +342,7 @@ def test_gcmpiengine_accepts_resource_specification(task_uuid, randomstring):
with mock.patch.object(GlobusMPIEngine, "_ExecutorClass") as mock_ex:
mock_ex.__name__ = "ClassName"
mock_ex.return_value = mock.Mock(launch_cmd="")
engine = GlobusMPIEngine()
engine = GlobusMPIEngine(address="127.0.0.1")
engine.submit(str(task_uuid), b"some task", resource_specification=spec)

assert engine.executor.submit.called, "Verify test: correct internal method invoked"
Expand Down

0 comments on commit 8b14b16

Please sign in to comment.