-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add env support and remove params from env #231
Conversation
This will make working with Hugging Face tokens easier. This would allow you to create a secret with the following content: ``` HUGGING_FACE_HUB_TOKEN: my-token ``` Afterwards all Substratus resources will be able to use this HuggingFace token. This also removes the need for custom entrypoints in the images that strip PARAM_ from the env variable.
@@ -314,9 +314,14 @@ func nbPodName(nb *apiv1.Notebook) string { | |||
} | |||
|
|||
// notebookPod constructs a Pod for the given Notebook. | |||
func (r *NotebookReconciler) notebookPod(notebook *apiv1.Notebook, model *apiv1.Model, dataset *apiv1.Dataset) (*corev1.Pod, error) { | |||
func (r *NotebookReconciler) notebookPod(ctx context.Context, notebook *apiv1.Notebook, model *apiv1.Model, dataset *apiv1.Dataset) (*corev1.Pod, error) { |
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 can remove the ctx here since it's technically not needed anymore after latest refactor. What you think? @nstogner
PR for updating the images: substratusai/images#45 . I will also start updating the examples |
Verified on Kind with this model:
|
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.
LGTM, Some small suggestions, feel free to merge either way
internal/controller/utils_test.go
Outdated
|
||
actual, err := resolveEnv(tc.input) | ||
if err != nil { | ||
t.Errorf("error with case %v: %v", tc.input, err) |
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.
Prefer to use assert
or require
pkg
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 for equality check below
Allow setting environment variables directly from the Substratus resources.
E.g.
This also removes the functionality of Substratus of setting
PARAM_
environment variables. So this will require updating our images and possibly examples.