Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Draft: create some apps from traitlet on startup #516

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

Draft: create some apps from traitlet on startup #516

wants to merge 13 commits into from

Conversation

Adam-D-Lewis
Copy link
Member

@Adam-D-Lewis Adam-D-Lewis commented Nov 6, 2024 •
edited
Loading

Reference Issues or PRs

Contains some example code of how to pass in jhub apps to be run when Jhub apps starts up. You pass in the apps as a traitlet. They should be created if they don't exist.
Related to nebari-dev/nebari#2803

What does this implement/fix?

Put a x in the boxes that apply

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds a feature)
  • Breaking change (fix or feature that would cause existing features not to work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • Other (please describe):

Testing

  • Did you test the pull request locally?
  • Did you add new tests?

Documentation

Access-centered content checklist

Text styling

  • The content is written with plain language (where relevant).
  • If there are headers, they use the proper header tags (with only one level-one header: H1 or # in markdown).
  • All links describe where they link to (for example, check the Nebari website).
  • This content adheres to the Nebari style guides.

Non-text content

  • All content is represented as text (for example, images need alt text, and videos need captions or descriptive transcripts).
  • If there are emojis, there are not more than three in a row.
  • Don't use flashing GIFs or videos.
  • If the content were to be read as plain text, it still makes sense, and no information is missing.

Any other comments?

@vercel Vercel
Copy link

vercel bot commented Nov 6, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
jhub-apps ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 18, 2024 6:07am
jhub-apps-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 18, 2024 6:07am

Adam-D-Lewis
Adam-D-Lewis commented Nov 7, 2024
View reviewed changes
jhub_apps/service/app.py Outdated Show resolved Hide resolved
@Adam-D-Lewis Adam-D-Lewis changed the title create apps on startup Draft of how we might create some apps from traitlet on startup Nov 7, 2024
@Adam-D-Lewis Adam-D-Lewis changed the title Draft of how we might create some apps from traitlet on startup Draft: create some apps from traitlet on startup Nov 7, 2024
Adam-D-Lewis
Adam-D-Lewis commented Nov 7, 2024
View reviewed changes
jhub_apps/config_utils.py Outdated
@@ -58,3 +60,9 @@ class JAppsConfig(SingletonConfigurable):
None,
help="Disallow a set of frameworks to avoid spinning up apps using those frameworks",
).tag(config=True)

startup_apps = List(
trait=Any, # TODO: Change this, use Instance() maybe or define a new type - https://traitlets.readthedocs.io/en/stable/defining_traits.html
Copy link
Member Author

@Adam-D-Lewis Adam-D-Lewis Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just put this dict here for testing. I want to re-use the JHubAppConfig pydantic model somehow ideally.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should accept git repos here alternatively

Adam-D-Lewis
Adam-D-Lewis commented Nov 7, 2024
View reviewed changes
jhub_apps/service/app.py Outdated

def instantiate_startup_apps(user_options_list: list[dict[str, Any]], username: str):
# instantiate custom apps
for user_options_dict in user_options_list:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still need to add logic to only create the apps if they don't exist.

@aktech
Copy link
Member

aktech commented Nov 7, 2024

Thanks for the PR @Adam-D-Lewis, other than the gotchas to look for (as mentioned above), you're on the right path I believe.

@Adam-D-Lewis Adam-D-Lewis force-pushed the create_apps_on_startup branch from 9a5a5d1 to 52381cc Compare December 16, 2024 22:25
@Adam-D-Lewis
Copy link
Member Author

A few questions:

  • App creation is tied to a user. Which user should be used to create the apps in this case?
  • Is it possible to lock an app so it can't be edited except by modifying the JAppsConfig.startup_apps traitlet? Maybe if we create it with some username that doesn't otherwise exist e.g. nebari-config.
  • How do you identify which servers are JHub Apps vs any other named server? Is there a HubClient method that can return only Jhub app named servers for a user?

@Adam-D-Lewis
Copy link
Member Author

Adam-D-Lewis commented Dec 17, 2024 •
edited
Loading

I talked with Amit about this.

A few questions:

  • App creation is tied to a user. Which user should be used to create the apps in this case?

Create a service account to do this.

  • Is it possible to lock an app so it can't be edited except by modifying the JAppsConfig.startup_apps traitlet? Maybe if we create it with some username that doesn't otherwise exist e.g. nebari-config.

yes, currently only the person who created the app can edit it so using a service account should accomplish this.

  • How do you identify which servers are JHub Apps vs any other named server? Is there a HubClient method that can return only Jhub app named servers for a user?

The user_options is saved as an attribute of a user's server. You should be able to get access to it through the get_users method of the jhub_client.

Adam-D-Lewis
Adam-D-Lewis commented Dec 18, 2024
View reviewed changes
jhub_apps/config_utils.py
@@ -49,12 +109,25 @@ class JAppsConfig(SingletonConfigurable):
help="The number of workers to create for the JHub Apps FastAPI service",
).tag(config=True)

allowed_frameworks = Bool(
allowed_frameworks = List(
Copy link
Member Author

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.

Copy link
Member

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.

Adam-D-Lewis
Adam-D-Lewis commented Dec 18, 2024
View reviewed changes
jhub_apps/service/utils.py
@@ -102,7 +106,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:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flyby: add return type annotation

Adam-D-Lewis
Adam-D-Lewis commented Dec 18, 2024
View reviewed changes
jhub_apps/service/utils.py
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)
Copy link
Member Author

@Adam-D-Lewis Adam-D-Lewis Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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

I am not sure how that would look like. That would make sense though, can you share an example?

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.

This should be done here:

jhub-apps/jhub_apps/configuration.py

Line 28 in a68df38

setattr(c.JAppsConfig, trait_name, defaults.get(trait_name))

You can instantiate the object there. I might not have followed traitlets best practices here.

Copy link
Member Author

Choose a reason for hiding this comment

The 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?

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

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aktech aktech aktech left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Adam-D-Lewis @aktech