Skip to content

Improve readability of host/platform eval code #53

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

Merged
merged 2 commits into from
Apr 13, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
4 changes: 2 additions & 2 deletions cylc/flow/hostuserutil.py
Original file line number Diff line number Diff line change
Expand Up @@ -197,8 +197,8 @@ def is_remote_host(self, name):

"""
if name not in self._remote_hosts:
if not name or name.split(".")[0].startswith("localhost"):
# e.g. localhost42.localdomain42
if not name or name.startswith("localhost"):
# e.g. localhost4.localdomain4
self._remote_hosts[name] = False
else:
try:
Expand Down
12 changes: 4 additions & 8 deletions cylc/flow/task_job_mgr.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,6 @@
)
from cylc.flow.pathutil import get_remote_workflow_run_job_dir
from cylc.flow.platforms import (
HOST_REC_COMMAND,
PLATFORM_REC_COMMAND,
get_host_from_platform,
get_install_target_from_platform,
get_localhost_install_target,
Expand Down Expand Up @@ -1120,14 +1118,12 @@ def _prep_submit_task_job(
host_n, platform_name = None, None
try:
if rtconfig['remote']['host'] is not None:
host_n = self.task_remote_mgr.subshell_eval(
rtconfig['remote']['host'], HOST_REC_COMMAND
host_n = self.task_remote_mgr.eval_host(
rtconfig['remote']['host']
)
else:
platform_name = self.task_remote_mgr.subshell_eval(
rtconfig['platform'],
PLATFORM_REC_COMMAND,
host_check=False
platform_name = self.task_remote_mgr.eval_platform(
rtconfig['platform']
)
except PlatformError as exc:
itask.waiting_on_job_prep = False
Expand Down
92 changes: 53 additions & 39 deletions cylc/flow/task_remote_mgr.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,14 @@
from subprocess import Popen, PIPE, DEVNULL
import tarfile
from time import sleep, time
from typing import Any, Deque, Dict, TYPE_CHECKING, List, NamedTuple, Tuple
from typing import (
Any, Deque, Dict, TYPE_CHECKING, List,
NamedTuple, Optional, Tuple
)

from cylc.flow import LOG
from cylc.flow.exceptions import PlatformError
import cylc.flow.flags
from cylc.flow.hostuserutil import is_remote_host
from cylc.flow.network.client_factory import CommsMeth
from cylc.flow.pathutil import (
get_dirs_to_symlink,
Expand All @@ -46,6 +48,8 @@
get_workflow_run_dir,
)
from cylc.flow.platforms import (
HOST_REC_COMMAND,
PLATFORM_REC_COMMAND,
NoHostsError,
PlatformLookupError,
get_host_from_platform,
Expand All @@ -67,6 +71,7 @@
)

from cylc.flow.loggingutil import get_next_log_number, get_sorted_logs_by_time
from cylc.flow.hostuserutil import is_remote_host

if TYPE_CHECKING:
from zmq.auth.thread import ThreadAuthenticator
Expand Down Expand Up @@ -106,74 +111,83 @@ def __init__(self, workflow, proc_pool, bad_hosts):
self.is_reload = False
self.is_restart = False

def subshell_eval(self, command, command_pattern, host_check=True):
"""Evaluate a task platform from a subshell string.

At Cylc 7, from a host string.
def _subshell_eval(
self, eval_str: str, command_pattern: re.Pattern
) -> Optional[str]:
"""Evaluate a platform or host from a possible subshell string.

Arguments:
command (str):
An explicit host name, a command in back-tick or $(command)
format, or an environment variable holding a hostname.
command_pattern (re.Pattern):
eval_str:
An explicit host/platform name, a command, or an environment
variable holding a host/patform name.
command_pattern:
A compiled regex pattern designed to match subshell strings.
host_check (bool):
A flag to enable remote testing. If True, and if the command
is running locally, then it will return 'localhost'.

Return (str):
Return:
- None if evaluation of command is still taking place.
- If command is not defined or the evaluated name is equivalent
to 'localhost', _and_ host_check is set to True then
'localhost'
- Otherwise, return the evaluated host name on success.
- 'localhost' if string is empty/not defined.
- Otherwise, return the evaluated host/platform name on success.

Raise PlatformError on error.

"""
# BACK COMPAT: references to "host"
# remove at:
# Cylc8.x
if not command:
if not eval_str:
return 'localhost'

# Host selection command: $(command) or `command`
Copy link
Owner

@wxtim wxtim Mar 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This docstring needs changing - it's no longer just the host, and if it's the platform the pattern shouldn't match `command` .

Because of the way you've generalized the function I'd be happy just deleting it.

match = command_pattern.match(command)
match = command_pattern.match(eval_str)
if match:
cmd_str = match.groups()[1]
if cmd_str in self.remote_command_map:
# Command recently launched
value = self.remote_command_map[cmd_str]
if isinstance(value, PlatformError):
raise value # command failed
elif value is None:
return # command not yet ready
else:
command = value # command succeeded
if value is None:
return None # command not yet ready
eval_str = value # command succeeded
else:
# Command not launched (or already reset)
self.proc_pool.put_command(
SubProcContext(
'remote-host-select',
['bash', '-c', cmd_str],
env=dict(os.environ)),
env=dict(os.environ)
),
callback=self._subshell_eval_callback,
callback_args=[cmd_str]
)
self.remote_command_map[cmd_str] = None
return self.remote_command_map[cmd_str]
return None

# Environment variable substitution
command = os.path.expandvars(command)
# Remote?
# TODO - Remove at Cylc 8.x as this only makes sense with host logic
if host_check is True:
if is_remote_host(command):
return command
else:
return 'localhost'
else:
return command
return os.path.expandvars(eval_str)

# BACK COMPAT: references to "host"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this part of the eval_host docstring.

Suggest noting that when we remove the back compat layer we re-merge eval_platform and _subshell_eval, else there will be a non purposeful layer of abstraction?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this part of the eval_host docstring.

I think it's fine to leave as a comment? It doesn't tell developers how to use the function which is what docstring is primarily for.

Suggest noting that when we remove the back compat layer we re-merge eval_platform and _subshell_eval, else there will be a non purposeful layer of abstraction?

I think that should be fairly self-evident when the time comes

# remove at:
# Cylc8.x
def eval_host(self, host_str: str) -> Optional[str]:
"""Evaluate a host from a possible subshell string.

Args:
host_str: An explicit host name, a command in back-tick or
$(command) format, or an environment variable holding
a hostname.

Returns 'localhost' if evaluated name is equivalent
(e.g. localhost4.localdomain4).
"""
host = self._subshell_eval(host_str, HOST_REC_COMMAND)
return host if is_remote_host(host) else 'localhost'

def eval_platform(self, platform_str: str) -> Optional[str]:
"""Evaluate a platform from a possible subshell string.

Args:
platform_str: An explicit platform name, a command in $(command)
format, or an environment variable holding a platform name.
"""
return self._subshell_eval(platform_str, PLATFORM_REC_COMMAND)

def subshell_eval_reset(self):
"""Reset remote eval subshell results.
Expand Down