-
Notifications
You must be signed in to change notification settings - Fork 54
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
Consolidate UI developer docs (#258) #323
Conversation
Resolves machine-learning-exchange#258 Signed-off-by: RRM123 <[email protected]>
Resolves machine-learning-exchange#258 Signed-off-by: RRM123 <[email protected]>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: RRM123 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@ckadner Forgot to tag you in this PR |
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.
Thanks @RRM123 -- this is a good improvement. I would make a few small changes.
dashboard/origin-mlx/README.md
Outdated
|
||
## UI Development with Docker Compose | ||
For information on how to get started with Docker Compose, and it's appropriate setup, check out the |
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 a good bit of information :-) I would change it slightly to:
For information on how to get started with Docker Compose before making any changes to the UI code, check out the Quick Start Guide and take a look at the docker-compose.yaml file to understand how the individual services like
mysql
,minio
,mlx-api
,mlx-ui
, etc. are working together.
dashboard/origin-mlx/README.md
Outdated
For information on how to get started with Docker Compose, and it's appropriate setup, check out the | ||
[Quick Start Guide](../../quickstart/README.md). | ||
|
||
To bring up the Docker Compose stack run: |
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 would remove the 2 commands here as they are duplicated from in the Quickstart guide, or, point out how they are different (i.e. using the --project-name ...
flag)
dashboard/origin-mlx/README.md
Outdated
```Bash | ||
docker compose --project-name mlx down | ||
``` | ||
To bring up the stack without the UI run: |
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.
these 2 commands are described already in context of the next passage
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.
Implemented changes outlined in the PR review.
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.
Thank you @RRM123 -- this is a good improvement to our UI developer docs. I think we can take it a few steps further even.
dashboard/origin-mlx/README.md
Outdated
@@ -72,15 +74,29 @@ docker push <your docker user-id>/<repo name>:<tag name> | |||
``` | |||
|
|||
## (Re-)Deploy to Kubernetes Cluster | |||
For information on how to deploy MLX on a Kubernetes Cluster or OpenShift on IBM Cloud, check out the guide [here](../../docs/mlx-setup.md). | |||
Once the clusters have been deployed, they will need to be redeployed for UI work. |
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 may be slightly misleading. A more precise phrasing would be:
Once the cluster has been deployed, the MLX UI will need to be redeployed after changes to the UI code have been made.
dashboard/origin-mlx/README.md
Outdated
|
||
Change the Docker image tag in the deployment spec server/mlx-ui.yml from image: ibmandrewbutler/open-ui:add-homepage to image: <your_docker_user_id>/<repo name>:<tag name> and then run: | ||
|
||
```Bash | ||
kubectl delete -f ./dashboard/origin-mlx/mlx-ui.yml | ||
kubectl apply -f ./dashboard/origin-mlx/mlx-ui.yml | ||
``` | ||
Change the image in /manifests/base/mlx-deployments/mlx-ui.yaml under the container with the name mlx-ui at spec.template.spec.containers |
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.
did you try this out? should this be either or? ... either
./dashboard/origin-mlx/mlx-ui.yml
or/manifests/base/mlx-deployments/mlx-ui.yaml
dashboard/origin-mlx/README.md
Outdated
|
||
## UI Development with Docker Compose | ||
For information on how to get started with Docker Compose before making any changes to the UI code, check out the [Quick Start Guide](../../quickstart/README.md) and take a look at the [docker-compose.yaml](../../quickstart/docker-compose.yaml) file to understand how the individual services like ```mysql```, ```minio```, ```mlx-api```, ```mlx-ui```, etc. are working together. | ||
|
||
The Docker Compose stack can be brought up and taken down by running the following commands. The ```--project-name``` tag takes in the following argument as the name of the docker compose stack, which can be viewed using [Docker Desktop](https://www.docker.com/products/docker-desktop/): |
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.
single back ticks ``
for inline code sections like
The
--project-name
flag ...
the benefit of using the --project-name
flag is to keep the docker compose network and the volumes (stored assets) separate from the quickstart for development. Each docker compose project has separate network and volumes
dashboard/origin-mlx/README.md
Outdated
@@ -116,7 +132,7 @@ docker compose rm -v -f | |||
docker volume prune -f | |||
``` | |||
|
|||
### Terminal 2 - Start the UI server | |||
### Terminal 2 - Start the MLX UI locally with Docker API |
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.
there is no Docker involved in this second terminal
dashboard/origin-mlx/README.md
Outdated
@@ -185,6 +201,9 @@ which matches a previous request and the difference of the time between the two | |||
|
|||
To invalidate or hard reset the cache, navigate to the settings page (clicking the three dots at the bottom of the sidebar) and click on the `Reset Cache` button. | |||
|
|||
# Development Guidlines: | |||
For information on UI code structure, design principles, etc. check out the [MLX Developer Guide](developer-guide.md). |
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.
"MLX UI Developer Guide" ... the API has a separate one
@@ -133,3 +77,5 @@ src/styles/ contains most (>90%) of the page styling in css. If the style needs | |||
|
|||
|
|||
src/lib/api/ contains all of the calls to the MLX API. If some API call is going wrong, it will likely be an issue in this folder. | |||
|
|||
For more information on UI Development Setup go [here](README.md) |
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.
if we have two files
- the
/dashboard/origin-mlx/README.md
and - the
/dashboard/origin-mlx/developer-guide.md
then we should make a clear separation of content:
- all the parts in the README.md that talk about development should be in the developer-guide,
- and the README only has information about how to use (and deploy) the MLX UI
Although the developer guide also needs to inform about how to redeploy the MLX UI. So I am not convinced we need two documents if the README can contain all of it. If we have one README, then it should flow from overview, architecture diagram, how to deploy (point to general deployment docs) to running locally to development with docker compose, rebuild Docker image, push it, redeploy just the new MLX UI image to existing cluster, ...
the DCO check is failing since some commits are missing the |
@ckadner I reworked the wording of the paragraphs in question, and I made sure to add a Signed-off-by to the new commits. How can I add it to the previous commits that are missing it? |
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
Thanks Rithik I made a few final updates and merged it directly.
* Add dev doc links to CONTRIBUTING.md (#312) Resolves a task from #304 Signed-off-by: Rafael Vasquez <[email protected]> * Fixes the references to latest kfctl release (#311) Resolves a task from #304 Signed-off-by: Rafael Vasquez <[email protected]> * Do not verify links in project dependencies (#320) Resolves #319 Signed-off-by: Christian Kadner <[email protected]> * Add navigation and description table of docs (#314) * Create readme for docs * Add description table to readme * Add links to table * update relative links * Change documentation to document Signed-off-by: Rafael Vasquez <[email protected]> * Add script to update the docs table (#317) Signed-off-by: Rafael Vasquez <[email protected]> * Modified the way sed version is set (#315) Instead of checking the operating system or shell emulator, test which version of `sed` is actually installed in the local environment. Resolves #301 Signed-off-by: Krishna Kumar <[email protected]> * Bump waitress from 2.0.0 to 2.1.1 in /api/server (#321) Bumps [waitress](https://github.com/Pylons/waitress) from 2.0.0 to 2.1.1. - [Release notes](https://github.com/Pylons/waitress/releases) - [Changelog](https://github.com/Pylons/waitress/blob/master/CHANGES.txt) - [Commits](Pylons/waitress@v2.0.0...v2.1.1) --- updated-dependencies: - dependency-name: waitress dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump jupyter-server from 1.13.4 to 1.15.4 in /api/server (#324) Bumps [jupyter-server](https://github.com/jupyter/jupyter_server) from 1.13.4 to 1.15.4. - [Release notes](https://github.com/jupyter/jupyter_server/releases) - [Changelog](https://github.com/jupyter-server/jupyter_server/blob/main/CHANGELOG.md) - [Commits](jupyter-server/jupyter_server@v1.13.4...v1.15.4) --- updated-dependencies: - dependency-name: jupyter-server dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump minimist from 1.2.5 to 1.2.6 in /dashboard/origin-mlx (#326) Bumps [minimist](https://github.com/substack/minimist) from 1.2.5 to 1.2.6. - [Release notes](https://github.com/substack/minimist/releases) - [Commits](https://github.com/substack/minimist/compare/1.2.5...1.2.6) --- updated-dependencies: - dependency-name: minimist dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump notebook from 6.4.8 to 6.4.10 in /api/server (#327) Bumps [notebook](http://jupyter.org) from 6.4.8 to 6.4.10. --- updated-dependencies: - dependency-name: notebook dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Update API developer docs (#325) * Add codegen workflow diagram * Describe codegen workflow * Describe API server package modules Closes #325 Signed-off-by: Pavan Pss <[email protected]> Co-authored-by: Christian Kadner <[email protected]> * Update UI developer docs (#323) Closes #323 Signed-off-by: RRM123 <[email protected]> * Correct description for make update_doc_table (#329) Signed-off-by: ezinneanne <[email protected]> * Update Kubernetes high version in deployment docs (#318) (#332) * Add Troubleshooting section * Limit K8s version for KIND cluster using `--image` flag Resolves #318 Signed-off-by: Kiran-Patel <[email protected]> Signed-off-by: Kiran Patel [email protected] Signed-off-by: Christian Kadner <[email protected]> Co-authored-by: Kiran-Patel <[email protected]> Co-authored-by: Christian Kadner <[email protected]> * fix errors in mlx-ui startup (#338) * Run mlx-ui as non-root user (#339) Closes #337 Signed-off-by: Christian Kadner <[email protected]> * Update MLX setup instructions for KF 1.5 (#346) * Update MLX setup instructions for KF 1.5 Signed-off-by: Christian Kadner <[email protected]> * fix selected assets show up in different menus (#342) Signed-off-by: Jiaxuan-Yang <[email protected]> * Consolidate readme (#356) Signed-off-by: Rafael Vasquez <[email protected]> Signed-off-by: Rafael Vasquez <[email protected]> Co-authored-by: Rafael Vasquez <[email protected]> * Document MLX Models Workshop (#352) Signed-off-by: Christian Kadner <[email protected]> * Add GitHub action to verify doc links (#357) Signed-off-by: Rafael Vasquez <[email protected]> Signed-off-by: Rafael Vasquez <[email protected]> Signed-off-by: Christian Kadner <[email protected]> Signed-off-by: Krishna Kumar <[email protected]> Signed-off-by: RRM123 <[email protected]> Signed-off-by: ezinneanne <[email protected]> Signed-off-by: Jiaxuan-Yang <[email protected]> Signed-off-by: Rafael Vasquez <[email protected]> Co-authored-by: Rafael Vasquez <[email protected]> Co-authored-by: Krishna Kumar Ramachandran <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Pavan Pss <[email protected]> Co-authored-by: RRM123 <[email protected]> Co-authored-by: Ezinne Anne Emilia <[email protected]> Co-authored-by: Kiran Patel <[email protected]> Co-authored-by: Kiran-Patel <[email protected]> Co-authored-by: jbusche <[email protected]> Co-authored-by: Joanna <[email protected]> Co-authored-by: Rafael Vasquez <[email protected]>
dashboard/origin-mlx/README.md
. Replaced with pointer todashboard/origin-mlx/README.md
.npm start
).dashboard/origin-mlx/developer-guide.md
for Development Guidlines.OLD_README.md