-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Support Pod Defaults in Tensorboard Web App #6924
Support Pod Defaults in Tensorboard Web App #6924
Conversation
aa491b6
to
fd76699
Compare
@kimwnasptd Please take a look |
Hi @surajkota, thank you for the PR. I 'm also working on KF's web apps and @kimwnasptd informed me about your PR. I 'd be happy to take a look at this and review this so I 'll start right away. I had also worked with Pod Defaults when creating the Notebook details page in JWA. |
Thanks @orfeas-k, please take a look when you get a chance |
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.
Hey sorry for the delay @surajkota. The PR looks great.
As a general comment, I noticed that you followed what we do in JWA for poddefaults which is great, since this is code that has been tested and we know it works. However, I think it is always a great practice to avoid maintaining the same code twice. For this reason, I suggest the following changes:
- Create a
lib-form-configurations
component in Kubeflow's common library and put this to use both in TWA and JWA - Create a
parser/poddefaults.py
file in the common code and put there a function that parses the PodDefaults and that both TWA and JWA will use.
Also, nit but I think renaming the PR to twa: Support Pod Defaults
would make it more easy to navigate to inside the repo's commits.
I 'll let @kimwnasptd comment on the changes regarding rbac
, since he has a greater overview of how they work.
for label in labels: | ||
cr_labels[label] = "true" | ||
|
||
return cr_labels |
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.
return cr_labels | |
return cr_labels | |
volumeMounts?: Array<V1VolumeMount>; | ||
volumes?: Array<V1Volume>; | ||
tolerations?: Array<V1Toleration>; | ||
} |
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.
} | |
} | |
Hi @orfeas-k, thanks for the feedback. I 100% agree with you on the above changes and I had started to do it, however I am on vacation right now without access to laptop and given the timeline for 1.7 release, can we merge it without the common lib changes and do it in a follow up PR post release? I can make the other nit changes suggested. I want the feature to be available in 1.7. Please let me know |
I'm good with merging this PR now and including it in the 1.7 RC, since this is the last feature we had in mind for this release for Notebooks WG. The feature also uses a lot of the same code from the JWA as well, so since we've tested it as well I'm confident that it should be OK. We'll also keep an eye for this for the remainder of the RC testing. I'll go on and merge this iteration so that we can include it in the next RC, cc @DomFleischmann Thanks for the work on this @surajkota! /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kimwnasptd The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Great, Thanks folks! |
* support Pod Defaults in Tensorboard Web App * flint files
* support Pod Defaults in Tensorboard Web App * flint files
* support Pod Defaults in Tensorboard Web App * flint files
* support Pod Defaults in Tensorboard Web App * flint files
* support Pod Defaults in Tensorboard Web App * flint files
* support Pod Defaults in Tensorboard Web App * flint files
Description of Changes
Add functionality to configure PodDefaults in the Tensorboard UI which allows users to configure tensorboard instance for using object store(e.g. endpoint, region in case of AWS S3), different service account etc.. This completes the feature requested in #6493. This is a follow up PR of #6874 and hence it will be great to merge it soon so it can be made available as part of 1.7 release. #6672
See user experience section for sample
Frontend changes:
Backend changes:
Manifest changes:
Testing
Tested using sample tensorboard event generated from - https://www.tensorflow.org/tensorboard/get_started#using_tensorboard_with_keras_modelfit and steps as described in user experience section below
Built an image using Make command to test the changes
Notes to reviewer
How is this file used? -
components/crud-web-apps/tensorboards/deploy/app-rbac.yaml
User Experience - use AWS S3 as object store to visualize events in Tensorboard
default-editor
as the service account. (Tensorboard controller creates instances usingdefault
service account by default). You can also use secrets but this example uses IAM role for Service AccountFollowing command adds S3 permissions to default-editor service account