I was looking at the documentation and the implementation of the options_form and options_from_form, on the example of the DockerSpawner, and the implementation right now feels like it is more complicated than necessary. I might be overlooking something, this is why I thought posting it as a question rather than an issue is a good start.
Right now in order to implement custom options one needs to do the following:
Define the form that reads these options
Parse the user provided options into the Spawner.user_options dictionary using a generic function.
Modify the behavior of the spawner.
dockerspawner.DockerSpawner itself has an implementation of options_form which:
Reads the "image" from the form and puts it into user_options
The start() method checks whether the image is in the list of allowed images and assigns self.image before proceeding with the rest of the actions.
Similarly, the DemoFormSpawner example parses the options and overrides LocalProcessSpawner.get_args and LocalProcessSpawner.get_env.
In other words, implementing options form requires implementing an arbitrarily powerful function as well as overriding spawner behavior by subclassing.
I wonder whether it makes sense to simplify using option forms by letting options_from_form generate the spawner config, which would be assigned to the spawner after being returned from options_from_form. This would allow to arbitrarily change the spawner configuration without subclassing, while still requiring subclassing to change the spawner behavior (just like it is now).
This could be done in a backwards-compatible way by implementing a new function config_from_form.
Does this idea make sense? Am I missing something?
It doesn’t seem that complicated, maybe I’m misunderstanding your point:
options_form(self): returns an html form
options_from_form(self, formdata): translate formdata into a dict and return that dict
I use pre_spawn_hook to convert user_options to spawner options, but you mention start. I’m not sure where I figured this out from, but I’m not surprised I don’t do it right!
Your response supports my point. I am not facing a difficulty to implement the options form, I rather say that doing so seems to require more work than it could.
The part that seems suboptimal is the need for step 3 in your list. Right now it requires subclassing the spawner. On the other hand, the most logical use for options form is to only assign to spawner configuration, which shouldn’t require subclassing.
To answer where I found .start: overriding the .start() method is recommended in the documentation, and this pattern is used in e.g. the DockerSpawner.
To make my proposal more concrete, I envision the implementation of Spawner.start to be changed to the equivalent of this code, which I now have in the subclass:
def start(self):
"""Indiscriminately update own config from form data and start.
This assumes that all validation is the responsibility of
options_from_form. The only exception is the allowed_images check,
which is already implemented in the start.
"""
for key, value in self.user_options.items():
setattr(self, key, value)
return super().start()
or a slightly more complicated but backwards-compatible analog of this.
I absolutely think the options form should get more love, thanks for bringing it up! The fancy profiles project is a good example.
The reason we don’t have complete pass-through user options is that users shouldn’t generally have permission to set arbitrary attributes on the spawner (e.g. resource limits, namespace, uid, etc.). It is also relevant to keep in mind that user_options can be set directly via an API spawn request, instead of needing to go through options_from_form (which only exists because in http forms, everything is a list of strings). That means that user_options may be fully controlled by the user, and needs to be treated as such, mainly that most if not all inputs need to be explicitly permitted by the deployment configuration, not default behavior.
I do think support for some amount of declarative config to:
render a simple form
process form data into user_options
process user_options into Spawner attributes
from a fairly simple schema (some folks may be shouting ‘jsonschema’ at this point, but I think that’s not appropriate for users to write).
One very basic version of this could be a mapping (or list!) of trait names to expose, and generate the form field and coercion based on the trait type, help string, etc. But I imagine you’d quickly want a bit more control than that. It would probably end up looking quite a bit like fancy-profiles.
Ah, thanks for the clarification! I didn’t appreciate that the API may specify arbitraryuser_options. As a side note, this is not quite clear in the docs.
I agree that schema-based processing would be better, but as a start what about the implementation below? That’s what I have right now, and if validate_user_options was configurable, I wouldn’t need a subclass.
allowed_images = {
"image_name": "image_value",
}
c.DockerSpawner.allowed_images = list(allowed_images.values())
class DockerSpawner(dockerspawner.DockerSpawner):
def options_from_form(self, formdata):
return {
"image": formdata.get("image", ["default"])[0],
"gpu": formdata.get("gpu", ["false"])[0] == "true",
"pull_image": formdata.get("pull_image", ["false"])[0] == "true",
}
def validate_user_options(self):
user_options = self.user_options
self.user_options = {
"image": allowed_images.get(user_options["image"], allowed_images["default"]),
"extra_host_config": (
{
**self.extra_host_config,
"device_requests": [
docker.types.DeviceRequest(count=-1, capabilities=[["gpu"]])
],
}
if user_options["gpu"]
else self.extra_host_config
),
"pull_policy": "always" if user_options["pull_image"] else "ifnotpresent",
}
def start(self):
"""Update own config from user_options and start.
This assumes that all validation is the responsibility of validate_user_options.
"""
self.validate_user_options()
for key, value in self.user_options.items():
setattr(self, key, value)
return super().start()
Yeah, a validate_user_options (maybe apply_user_options) that could be specified via config should obviate the need for a subclass. We could even allow it to be a Dict in which case it would be a mapping from user_option to trait name, which would enable declarative config.
I also think a defaultoptions_from_from that just does the basics of what you have there (get the first value of list as a string, maybe handle booleans, probably not numbers) would also go a long way toward being able to use simple forms from declarative config. It can’t handle forms in general (lists, numbers, etc.), but it’s probably useful enough.
. As a side note, this is not quite clear in the