-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add support for kernel launch parameters #22
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,7 @@ | |
port_names = ['shell_port', 'iopub_port', 'stdin_port', 'control_port', | ||
'hb_port'] | ||
|
||
|
||
class SubprocessKernelLauncher: | ||
"""Launch kernels in a subprocess. | ||
|
||
|
@@ -44,13 +45,14 @@ class SubprocessKernelLauncher: | |
""" | ||
transport = 'tcp' | ||
|
||
def __init__(self, kernel_cmd, cwd, extra_env=None, ip=None): | ||
def __init__(self, kernel_cmd, cwd, extra_env=None, ip=None, launch_params=None): | ||
self.kernel_cmd = kernel_cmd | ||
self.cwd = cwd | ||
self.extra_env = extra_env | ||
if ip is None: | ||
ip = localhost() | ||
self.ip = ip | ||
self.launch_params = launch_params | ||
self.log = get_app_logger() | ||
|
||
if self.transport == 'tcp' and not is_local_ip(ip): | ||
|
@@ -138,18 +140,29 @@ def format_kernel_cmd(self, connection_file, kernel_resource_dir=None): | |
# but it should be. | ||
cmd[0] = sys.executable | ||
|
||
ns = dict(connection_file=connection_file, | ||
# Preserve system-owned substitutions by starting with launch params | ||
ns = dict() | ||
if isinstance(self.launch_params, dict): | ||
ns.update(self.launch_params) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to drop all launch_params into this namespace, or have a sub-dict for that and leave room in case there's a need for other kinds of parameters in the future? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I see your point relative to another area I was thinking about - environment variables - where the metadata also includes a list of supported environment variables. We could then maintain separate dicts. That said, having everything in the same namespace is only an issue if there are conflicts between env names and launch args and I'd be okay with stating that those spaces are shared. I think we could keep a single set of parameters by adding meta-property indicating their expected usages. For example, a I think this comes down to extensibility and my feeling is it would be better to extend the schema with an additional context type vs. supporting a varying number of sub-dicts - where each sub-dict infers its context. That said, my preference toward a metadata approach is just slightly stronger. |
||
|
||
# Add system-owned substitutions | ||
ns.update(dict(connection_file=connection_file, | ||
prefix=sys.prefix, | ||
) | ||
)) | ||
|
||
if kernel_resource_dir: | ||
ns["resource_dir"] = kernel_resource_dir | ||
|
||
pat = re.compile(r'{([A-Za-z0-9_]+)}') | ||
|
||
def from_ns(match): | ||
"""Get the key out of ns if it's there, otherwise no change.""" | ||
return ns.get(match.group(1), match.group()) | ||
"""Get the key out of ns if it's there, otherwise no change. | ||
Return as string since that's what is required by pattern | ||
matching. We know this should be safe currently, because | ||
only 'connection_file', 'sys.prefix' and 'resource_dir' are | ||
candidates - all of which are strings. | ||
""" | ||
return str(ns.get(match.group(1), match.group())) | ||
|
||
return [pat.sub(from_ns, arg) for arg in cmd] | ||
|
||
|
@@ -310,12 +323,12 @@ def prepare_interrupt_event(env, interrupt_event=None): | |
env["IPY_INTERRUPT_EVENT"] = env["JPY_INTERRUPT_EVENT"] | ||
return interrupt_event | ||
|
||
def start_new_kernel(kernel_cmd, startup_timeout=60, cwd=None): | ||
def start_new_kernel(kernel_cmd, startup_timeout=60, cwd=None, launch_params=None): | ||
"""Start a new kernel, and return its Manager and a blocking client""" | ||
from ..client import BlockingKernelClient | ||
cwd = cwd or os.getcwd() | ||
|
||
launcher = SubprocessKernelLauncher(kernel_cmd, cwd=cwd) | ||
launcher = SubprocessKernelLauncher(kernel_cmd, cwd=cwd, launch_params=launch_params) | ||
connection_info, km = launcher.launch() | ||
kc = BlockingKernelClient(connection_info, manager=km) | ||
try: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably say this level of the API should have specific kwargs which the provider is responsible for extracting from the generic
launch_params
. E.g. this could be calledargv_format_args
or something.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I first read this, I figured you were stating that these three keyword parameters could be collapsed into **kwargs. But now that I look closer, it seems like your comment really applies to the code that constructs the
SubprocessKernelLauncher
instance, since that would be location in which argv-only parameters could be extracted intoargv_format_args
. Or are you saying that this method would have additional code that pulls the argv-only parameters intoargv_format_args
, for example?I don't mind replacing this single parameter with **kwargs, but I'd still need to add code that pulls
launch_params
out ofkwargs
making those values available to the format code until the metadata definition is formalized. Or update the callers to use a different keyword.Could you please clarify what you mean? Sorry.