Add support for kernel launch parameters #22
Conversation
| # 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.
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.
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 context meta-property could be included that has one of a set of values like {'env', 'argv', 'config'}. The presentation layer could choose to use the context to split how it prompts for the values or not - its up to them. The provider could decompose list into multiple sets as it chooses.
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.
| @@ -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): | |||
There was a problem hiding this comment.
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 called argv_format_args or something.
There was a problem hiding this comment.
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 into argv_format_args. Or are you saying that this method would have additional code that pulls the argv-only parameters into argv_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 of kwargs 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.
I kind of like the list-of-dicts design. Not a strong preference, but if you're going to generate documentation or an interactive form from it, you want to write an ordered collection. I know recent Python will probably preserve the order, but JSON objects are defined as unordered, and you may want to reserialise it and send it to some other code. |
|
Good point about ordered dicts. The reason I prefer metadata be a dict of dicts is for correlation in the launch methods of the provider. In order to validate parameters, the client-provided parameter should be easily found in the corresponding metadata. By using the parameter name as the key to its metadata, this is quick and easy; much more so that scanning each element asking if it's |
Codecov Report
@@ Coverage Diff @@
## master #22 +/- ##
==========================================
+ Coverage 70.95% 71.07% +0.11%
==========================================
Files 27 27
Lines 2045 2043 -2
==========================================
+ Hits 1451 1452 +1
+ Misses 594 591 -3
Continue to review full report at Codecov.
|
|
After experimenting with JSON schema, I think the best approach would be for the launch parameters to be defined in native JSON schema. (Originally, I was thinking that we would build a schema so we could entertain custom properties, but I think pure JSON schema is more familiar and tools would adhere to it better. In addition, parameter validation would be more straight forward. I've updated the test to use this to define the launch_parameters: "metadata": {
"launch_parameter_schema": {
"title": "Params_kspec Kernel Provider Launch Parameter Schema",
"properties": {
"line_count": {"type": "integer", "minimum": 1, "default": 20, "description": "The number of lines totail"},
"follow": {"type": "string", "enum": ["-f", "-F"], "default": "-f", "description": "The follow option to tail"},
"cpus": {"type": "number", "minimum": 0.5, "maximum": 8.0, "default": 4.0, "description": "The number of CPUs to use for this kernel"},
"memory": {"type": "integer", "minimum": 2, "maximum": 1024, "default": 8, "description": "The number of GB to reserve for memory for this kernel"}
},
"required": ["line_count", "follow"]
}
} |
|
Just remembered that Kyle might be interested in this as well: @rgbkrk |
👍 for defining in native JSON Schema. You probably haven't been following the Jupyter Telemetry or Metadata work, but defining JSON schemas is foundational to this work. It would be awesome to hook the kernel provider work into Telemetry and Metadata systems. We could easily emit/store and validate kernel provider events using your schema. |
|
Feel free to resolve the conflicts and merge this. |
This updates the launch methods to include an optional parameter consisting of a dictionary of name/value pairs. It also applies these to the SubprocessKernelLauncher's cmd string. Finally, it provides a POC for how parameter metadata may get expressed and processed. Fixes #19
These changes add the ability for kernel provider clients to pass in "launch" parameters. I chose "launch parameters" over "kernel parameters" because many of the parameters that a client will provide will be used to seed the environment in which the kernel will run, and not necessarily be conveyed to the kernel directly.
More importantly than the
launch_paramsargument added to thelaunch()methods, the test code illustrates how the metadata corresponding to the available parameters could be implemented. In this case, the provider is a Spec provider, so the metadata is pulled directly from the kernelspec. Other providers may choose to do things differently, but the end result is that themetadataentry returned fromfind_kernels()would include alaunch_params(or whatever we call it) stanza consisting of a list of dicts where each element is a parameter and its dict describes that parameter.<aside>
We may instead want a dict of dicts where the key is the parameter's symbolic_name (i.e., the canonical value used in substitution) and the value is a dict of the metadata - including a more formal display_name (used in presentation). I kinda like that better, but that's more outside the scope of this PR.
</aside>
Here's the example spec used in the test code...
The client (front-end) would then use the metadata to prompt for values and create a simple dictionary of name, value pairs. These then get re-validated by the provider against the metadata. Required values that were not provided but have specified defaults are set, ranges are checked (as appropriate), etc. Once validated, the parameters are used in whichever way the provider has defined.
Copying a few other folks that might be interested: @SylvainCorlay @Zsailer @lresende @rolweber @jasongrout @blink1073
EDIT: replaced previously custom schema with JSON schema referenced in comment below (and added Steve to cc list).