-
Notifications
You must be signed in to change notification settings - Fork 1
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
DM-37758: Replace Cachemachine with JupyterLab Controller #199
Conversation
"image_dropdown": "use_image_from_dropdown", | ||
"size": self.config.image_size, | ||
"image_list": [image.path], | ||
"image_dropdown": [image.path], # Not used |
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.
Does this have to be sent? It would be nice if it could be omitted since it's not used.
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 think I can modify the models on the receiving end to make those optional. At least one of list or dropdown needs to be sent, but I can certainly send dropdown as null.
def to_camel_case(string: str) -> str: | ||
"""Convert a string to camel case. | ||
|
||
Originally written for use with Pydantic as an alias generator so that the | ||
model can be initialized from camel-case input (such as Kubernetes | ||
objects). | ||
|
||
Parameters | ||
---------- | ||
string | ||
Input string | ||
|
||
Returns | ||
------- | ||
str | ||
String converted to camel-case with the first character in lowercase. | ||
""" | ||
components = string.split("_") | ||
return components[0] + "".join(c.title() for c in components[1:]) | ||
|
||
|
||
class CamelCaseModel(BaseModel): | ||
"""This is what we will use in place of BaseModel for the Spawner | ||
Pydantic models. Any configuration can be given in Helm-appropriate | ||
camelCase, but internal Python methods and objects will all be snake_case. | ||
|
||
This isn't actually all that useful here, but since these models are | ||
copied from jupyterlabcontroller, which *does* make use of these features, | ||
it's easier than messing with the models. | ||
""" | ||
|
||
class Config: | ||
"""Pydantic configuration.""" | ||
|
||
alias_generator = to_camel_case | ||
allow_population_by_field_name = True |
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 don't think you end up needing any of this code since you use dashify instead.
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.
ControllerImages is a CamelCase model and I wanted to minimize the changes when I forklifted the model from jupyterlabcontroller.
That is a question I've got, actually: Ideally models shared between applications (e.g. as producer and consumer, which is what we're doing here) should go in some common library they both reference. What's a good pattern for that?
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've been pondering the same thing for Pydantic-based Avro for Roundtable Kafka producers and consumers. I was thinking about putting them in Safir?
class SpawnerEnum(str, Enum): | ||
"""This will validate that the name is entirely upper case, and | ||
will produce auto() values in lower case with underscores turned to | ||
dashes. | ||
""" | ||
|
||
RECOMMENDED = "recommended" | ||
LATEST_WEEKLY = "latest-weekly" | ||
BY_REFERENCE = "by-reference" | ||
def _generate_next_value_( # type: ignore | ||
name, start, count, last_values | ||
) -> str: | ||
if name != name.upper(): | ||
raise RuntimeError("Enum names must be entirely upper-case") | ||
return dashify(name.lower()) | ||
|
||
|
||
class JupyterImage(BaseModel): | ||
"""Represents an image to spawn as a Jupyter Lab.""" | ||
class NubladoEnum(str, Enum): | ||
"""This will validate that the name is entirely upper case, and | ||
will produce auto() values in lower case. This is exactly StrEnum from | ||
Python 3.11, except for the validation step.""" | ||
|
||
reference: str = Field( | ||
def _generate_next_value_( # type: ignore | ||
name, start, count, last_values | ||
) -> str: | ||
if name != name.upper(): | ||
raise RuntimeError("Enum names must be entirely upper-case") | ||
return name.lower() |
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 don't understand these changes. They seem like a ton of unnecessary complexity. Why can't we just use simple enums?
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.
Same as above: I tried to minimize the changes when pulling this over from JupyterLab Controller. We do care about the actual string values there. I think the right way to go may be to go to Python 3.11 and use its new StrEnum class directly, though. That doesn't get us the keys-are-uppercase validator, but, meh.
digest: Optional[str] = Field( | ||
digest: str = Field( | ||
..., | ||
title="Hash of the last layer of the Docker container", | ||
description="May be null if the digest isn't known", | ||
name="digest", | ||
example=( | ||
"sha256:419c4b7e14603711b25fa9e0569460a753c4b2449fe275bb5f89743b" | ||
"01794a30" | ||
"sha256:e693782192ecef4f7846ad2b21" | ||
"b1574682e700747f94c5a256b5731331a2eec2" | ||
), | ||
title="unique digest of image contents", | ||
) |
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.
This should be optional since we don't care about it, I think. Then when the image is manually constructed for the case where we explicitly provide an image, we don't need to pass in a dummy string and can just let it default to None
.
class JupyterImageClass(SpawnerEnum): | ||
"""Possible ways of selecting an image.""" | ||
|
||
RECOMMENDED = auto() | ||
LATEST_WEEKLY = auto() | ||
BY_REFERENCE = auto() |
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.
class JupyterImageClass(SpawnerEnum): | |
"""Possible ways of selecting an image.""" | |
RECOMMENDED = auto() | |
LATEST_WEEKLY = auto() | |
BY_REFERENCE = auto() | |
class JupyterImageClass(Enum): | |
"""Possible ways of selecting an image.""" | |
RECOMMENDED = "recommended" | |
LATEST_WEEKLY = "latest-weekly" | |
BY_REFERENCE = "by-reference" |
@@ -203,8 +203,7 @@ async def test_long_error( | |||
"jupyter": { | |||
"image_class": "by-reference", | |||
"image_reference": ( | |||
"registry.hub.docker.com/lsstsqre/sciplat-lab" | |||
":d_2021_08_30" | |||
"docker.io/lsstsqre/sciplat-lab" ":d_2023_02_03" |
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.
Strings can be combined now.
image = JupyterImage.from_reference(self.config.image_reference) | ||
ref = self.config.image_reference | ||
image = ControllerImage( | ||
path=ref, name=ref, digest="unknown", tags={} |
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.
digest
and tags
should be optional in the model so that you can just omit them here.
class ControllerImages(CamelCaseModel): | ||
recommended: Optional[ControllerImage] = None | ||
latest_weekly: Optional[ControllerImage] = None | ||
latest_daily: Optional[ControllerImage] = None | ||
latest_release: Optional[ControllerImage] = None | ||
all: List[ControllerImage] = Field(default_factory=list) | ||
|
||
class Config: | ||
alias_generator = dashify | ||
allow_population_by_field_name = True |
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.
This is defined as a CamelCaseModel
but then you override everything CamelCaseModel
does, so this should just be BaseModel
.
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.
This comes back to the shared-class thing. It makes sense for it to be a CamelCaseModel in the JupyterLab Controller, because we instantiate it directly from a CamelCase YAML file. It doesn't really make sense here, but I also feel like it should be as nearly as possible the same model in each place, or we should have a repository or set of repositories that hold models that are used as things we're passing around over the network.
class PartialImage(CamelCaseModel): | ||
path: str = Field( |
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.
Not sure why this isn't merged with ContainerImage
. I don't think mobu ever cares about PartialImage
.
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.
One more "well, I wanted to leave the classes from the controller intact."
tags: Dict[str, str] = Field( | ||
..., | ||
name="tags", | ||
title="Map between tag and its display name", | ||
) |
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.
Should be optional so that you don't need to supply it when constructing the image by reference.
This was done using the bot API instead of talking to the lab controller directly in #227. |
No description provided.