-
Notifications
You must be signed in to change notification settings - Fork 0
feat: revise Models documentation and add Model Artifact management s… #53
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
Conversation
Summary of ChangesHello @ZhangEnYao, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request expands the documentation for managing AI/ML models within the OtterScale cluster. It updates the existing 'Models' page to reflect a broader scope beyond just LLMs and introduces a dedicated section for managing model artifacts, providing comprehensive guidance on their lifecycle from creation to deletion. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request significantly revises the documentation for the "Models" page and adds a new page for "Model Artifacts". The updates provide more detailed and structured information, which is a great improvement. My review includes suggestions to enhance clarity on a few potentially confusing table column descriptions and to correct minor formatting inconsistencies for better readability. Overall, these are good changes to improve the project's documentation.
| | **Name** | The name of the model. | | ||
| | **Model Name** | The unique identifier of the model (modelName/id). | |
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.
The distinction between Name and Model Name is unclear. Name is described as "The name of the model," while Model Name is "The unique identifier of the model (modelName/id)." This could be confusing for users. Please clarify the difference. For example, is Name a user-friendly display name, while Model Name is the technical identifier used by the system? Providing a more distinct description and an example would be helpful.
| **Name** | A user-friendly display name for the model. |
| **Model Name** | The unique technical identifier of the model (e.g., `llama3-8b-instruct`). |
|
|
||
| The dashboard integrates with Prometheus to provide real-time metrics for each model: | ||
|
|
||
| - **GPU & KV Cache**: Monitor these percentages to ensure your models are not running out of memory context, which could degrade performance or cause errors. | ||
| - **Latency Metrics**: Track "Requests" and "Time to First Token" to ensure the models are responsive and meeting service level objectives (SLOs). | ||
|
|
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 are several formatting inconsistencies in the file:
- Multiple consecutive blank lines are used here and on lines 52-53. A single blank line is sufficient for separating sections and improves source readability.
- Line 37 contains trailing whitespace (
...if available.). This should be removed if a hard line break is not intended.
Please clean up these minor formatting issues to improve the document's consistency.
| | **Ready** | Number of ready containers vs total containers. | | ||
| | **Restarts** | Number of times containers in the pod have restarted. | | ||
| | **Conditions** | The most recent condition or error status for the pod. | | ||
| | **Time to First Token** | The sum of time (in seconds) taken to generate the first token for requests to this pod. | |
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.
The description for Time to First Token states it is the "sum of time". This is an unusual metric for latency, as a sum would grow indefinitely and not provide a clear snapshot of current performance. Typically, latency is measured as an average, median, or percentile (e.g., p95). Please verify if "sum" is correct. If so, consider adding an explanation. If it's an average or another statistical measure, please update the description accordingly.
| **Time to First Token** | The average time (in seconds) taken to generate the first token for requests to this pod. |
| import { Steps, Aside } from '@astrojs/starlight/components'; | ||
|
|
||
|
|
||
|
|
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.
| | **Status** | The current job status for downloading or processing the artifact. | | ||
| | **Phase** | The current phase/status of the artifact. | |
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.
The descriptions for Status and Phase are very similar and could be confusing. Status is "The current job status for downloading or processing the artifact," and Phase is "The current phase/status of the artifact." Please clarify the distinction between these two columns. For example, does Status refer to a background job (like Pending, Running, Succeeded) while Phase refers to the artifact's lifecycle state (like Downloading, Ready, Error)? Providing examples for each would be very helpful.
| **Status** | The status of the background job processing the artifact (e.g., `Pending`, `Running`, `Succeeded`). |
| **Phase** | The lifecycle phase of the artifact itself (e.g., `Downloading`, `Ready`, `Error`). |
…ection