-
Notifications
You must be signed in to change notification settings - Fork 12
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
Draft: create some apps from traitlet on startup #516
base: main
Are you sure you want to change the base?
Changes from all commits
3620828
292cbdc
52381cc
23fb2c2
e8c7582
aee7385
db648c6
b8f8c12
2f6b374
48c9aa5
8cb35ff
4faf955
74c675a
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 | ||
---|---|---|---|---|
|
@@ -11,6 +11,7 @@ | |||
from jupyterhub.app import JupyterHub | ||||
from traitlets.config import LazyConfigValue | ||||
|
||||
from jhub_apps.config_utils import JAppsConfig | ||||
from jhub_apps.hub_client.hub_client import HubClient | ||||
from jhub_apps.service.models import UserOptions | ||||
from jhub_apps.spawner.types import FrameworkConf, FRAMEWORKS_MAPPING, FRAMEWORKS | ||||
|
@@ -28,6 +29,8 @@ def get_jupyterhub_config(): | |||
jhub_config_file_path = os.environ["JHUB_JUPYTERHUB_CONFIG"] | ||||
logger.info(f"Getting JHub config from file: {jhub_config_file_path}") | ||||
hub.load_config_file(jhub_config_file_path) | ||||
# hacky, but I couldn't figure out how to get validation of the config otherwise (In this case, validation converts the dict in the config to a Pydantic model) | ||||
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 wasn't very familiar with traitlets before this. It seems strange that we don't have an JHubApp traitlets.Application that would get instantiated. The traitlets.config.loader.Config objects don't seem to get validated/coerced until instantiation. I needed c.JAppsConfig.startup_apps to be converted from list[dict] to list[BaseModel]. This was a hacky work around, but I'm open to better ways to do this if you can think of any. 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 am not sure how that would look like. That would make sense though, can you share an example?
This should be done here: jhub-apps/jhub_apps/configuration.py Line 28 in a68df38
You can instantiate the object there. I might not have followed traitlets best practices here. 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'm certainly not an expert in traitlets either, but the example I was thinking of was conda store where their fastapi server is a traitlets Application. See here 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. Amit meant to post that the JHUBAppConfig should be instantiated in install_jhub_apps method. |
||||
hub.config.JAppsConfig.startup_apps = JAppsConfig(config=hub.config).startup_apps | ||||
config = hub.config | ||||
logger.info(f"JHub Apps config: {config.JAppsConfig}") | ||||
return config | ||||
|
@@ -102,7 +105,7 @@ async def get_spawner_profiles(config, auth_state=None): | |||
) | ||||
|
||||
|
||||
def encode_file_to_data_url(filename, file_contents): | ||||
def encode_file_to_data_url(filename, file_contents) -> str: | ||||
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. flyby: add return type annotation |
||||
"""Converts image file to data url to display in browser.""" | ||||
base64_encoded = base64.b64encode(file_contents) | ||||
filename_ = filename.lower() | ||||
|
@@ -117,7 +120,7 @@ def encode_file_to_data_url(filename, file_contents): | |||
return data_url | ||||
|
||||
|
||||
def get_default_thumbnail(framework_name): | ||||
def get_default_thumbnail(framework_name) -> str: | ||||
framework: FrameworkConf = FRAMEWORKS_MAPPING.get(framework_name) | ||||
thumbnail_path = framework.logo_path | ||||
return encode_file_to_data_url( | ||||
|
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.
Flyby: seems like these should be Lists not Bools, but I could be wrong.
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.
Yes correct. Thanks for spotting this.