[bugfix] fix model agent progress logging, deletion, and error handling#121
Conversation
- Fix progress logging by reordering HubConfig options to prevent logger override - Fix model deletion using consistent path calculation with getDestPath - Add proper nil checks for progressManager.logger to prevent panics - Add HTTP 429 rate limiting handling with exponential backoff - Enable detailed progress logging for model downloads - Fix RBAC permissions to allow model-agent to get secrets
slin1237
requested review from
CatherineSue,
pallasathena92 and
zhyncs
as code owners
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @slin1237, 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 significantly enhances the robustness and functionality of the model agent by resolving several long-standing issues. It improves the reliability of model downloads and deletions, makes the agent more resilient to external API rate limits, and refines Kubernetes resource configurations for better secret management and permissions.
Highlights
- Model Agent Stability & Reliability: Fixed critical issues related to model download progress logging, ensuring consistent and visible progress updates. Addressed nil pointer dereferences to prevent panics, improving overall stability.
- Robust Model Deletion: Resolved a bug where model files were not properly removed from the filesystem due to inconsistent path construction between download and deletion operations, ensuring clean cleanup.
- Improved External API Handling: Implemented comprehensive handling for HTTP 429 (Too Many Requests) errors from the Hugging Face API, including retry logic with exponential backoff and respect for the
Retry-Afterheader, making the agent more resilient to rate limiting. - Kubernetes Permissions & Secret Management: Updated RBAC permissions to allow the model-agent service account to
getsecrets at the cluster scope. Corrected the namespace lookup forClusterBaseModelsecrets to ensure proper retrieval of Hugging Face tokens. - Model Definition Updates: Standardized Hugging Face token key references in
ClusterBaseModeldefinitions and performed minor cleanup of metadata in some model configurations.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces several important bug fixes and improvements to the model agent. Key changes include fixing progress logging by reordering configuration options, ensuring consistent model pathing for deletion, adding nil-pointer checks for the logger, and implementing robust HTTP 429 rate-limiting handling with retries. It also corrects RBAC permissions for accessing secrets and fixes a namespace issue for ClusterBaseModel resources. The changes are well-described and address critical production issues. The code quality is generally high, with good use of context for cancellation and clear error handling. I've found one minor issue in a log message format that should be addressed.
| secret, err := s.kubeClient.CoreV1().Secrets(namespace).Get(context.Background(), *baseModelSpec.Storage.StorageKey, metav1.GetOptions{}) | ||
| if err != nil { | ||
| s.logger.Warnf("Failed to retrieve secret %s for Hugging Face token: %v", *baseModelSpec.Storage.StorageKey, err) | ||
| s.logger.Warnf("Failed to retrieve secret %s in namespace %s for Hugging Face token: %v. Opc-Request-Id: %v", *baseModelSpec.Storage.StorageKey, namespace, err, nil) |
There was a problem hiding this comment.
The logging for a failed secret retrieval has been improved to include the namespace, which is great for debugging. However, it also adds an Opc-Request-Id field that is hardcoded to nil. This adds noise to the logs and can be misleading. Since there's no request ID available in this context, this part of the log message should be removed.
| s.logger.Warnf("Failed to retrieve secret %s in namespace %s for Hugging Face token: %v. Opc-Request-Id: %v", *baseModelSpec.Storage.StorageKey, namespace, err, nil) | |
| s.logger.Warnf("Failed to retrieve secret %s in namespace %s for Hugging Face token: %v", *baseModelSpec.Storage.StorageKey, namespace, err) |
…ng (#121) - Fix progress logging by reordering HubConfig options to prevent logger override - Fix model deletion using consistent path calculation with getDestPath - Add proper nil checks for progressManager.logger to prevent panics - Add HTTP 429 rate limiting handling with exponential backoff - Enable detailed progress logging for model downloads - Fix RBAC permissions to allow model-agent to get secrets
1 participant
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR fixes several critical issues in the model agent:
Progress Logging: Fixed progress logging for model downloads which was not working due to logger being cleared by WithViper option. Reordered HubConfig options to apply WithViper first, then WithLogger.
Model Deletion: Fixed model deletion failure where files were not being removed from filesystem. The deletion code was constructing a different path than the download code (case mismatch). Now both use the same
getDestPathfunction.Nil Pointer Dereference: Added proper nil checks for
progressManager.loggerthroughout the codebase to prevent panics when logger is not initialized.HTTP 429 Rate Limiting: Added proper handling for HTTP 429 (Too Many Requests) errors from Hugging Face API with:
RBAC Permissions: Added permission for model-agent service account to get secrets at cluster scope for accessing HF tokens.
Namespace Issue: Fixed ClusterBaseModels to look for secrets in "ome" namespace instead of empty string.
Which issue(s) this PR fixes:
Fixes # (no specific issue, but addresses multiple production issues)
Special notes for your reviewer:
make fmt,make vet,make tidy, andmake testDoes this PR introduce a user-facing change?