-
Notifications
You must be signed in to change notification settings - Fork 1
Wave3 #8
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
This pull request introduces significant enhancements to the model publishing workflow, including a detailed requirements document, an implementation plan, and updates to the authentication system. It also removes outdated configurations and adds new dependencies for better functionality. Below is a summary of the most important changes grouped by theme. Enhancements to Model Publishing Workflow:
Authentication System Updates:
Removal of Outdated Configurations:
Minor Changes:
These changes collectively improve the functionality, security, and scalability of the model publishing workflow while simplifying the codebase and documentation. |
Added 'configmaps' to the list of resources and enabled the 'watch' verb for core resources in the management.yaml RBAC configuration. This allows for broader monitoring and management capabilities.
This pull request introduces significant updates to the model publishing workflow, including new requirements documentation, implementation plans, and enhancements to authentication mechanisms. It also includes smaller changes to configuration files and documentation. Below is a summary of the most important changes: Model Publishing Workflow EnhancementsRequirements Documentation:
Implementation Plan:
Authentication Enhancements
Configuration Updates
Documentation Updates
Minor Code Changes
|
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.
Pull Request Overview
This PR implements a full end-to-end model publishing workflow, enhances authentication with API keys, removes deprecated management configs, and updates documentation.
- Adds detailed publishing specs, tasks, and UI components for guided and quick publish flows
- Introduces
PublishingService
with routes for publish, unpublish, key rotation, and validation - Cleans up old Kubernetes manifests and updates
management-utils
, server registration, and documentation
Reviewed Changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
scripts/retest.sh | New test script to redeploy and port-forward the management service |
scripts/build-and-push-images.sh | Removed deprecated manifest update helper |
management/utils.go | Simplified command execution and YAML conversion, switched to yaml.v2 |
management/ui/src/index.css | Added CSS for publishing UI components and workflow indicators |
management/ui/src/contexts/ApiContext.js | Exposed model publishing and API key management endpoints |
management/types.go | Defined new types for publish configs, published models, and docs |
management/server.go | Registered publishing routes and initialized PublishingService |
management/publishing.go | Core implementation of the publishing workflow |
Comments suppressed due to low confidence (1)
management/publishing.go:44
- New core publishing logic is substantial and currently lacks unit or integration tests. Consider adding tests covering error paths, successful publish, and rollback scenarios.
func (s *PublishingService) PublishModel(c *gin.Context) {
const params = namespace ? { namespace } : {}; | ||
return api.post(`/models/${modelName}/publish/rotate-key`, {}, { params }); | ||
}, | ||
validateAPIKey: (apiKey) => api.post('/validate-api-key', { apiKey }), |
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 client is sending the API key in the request body but the server handler reads it from headers. Either adjust the client to set the X-API-Key header or update the server to parse the JSON body.
validateAPIKey: (apiKey) => api.post('/validate-api-key', { apiKey }), | |
validateAPIKey: (apiKey) => api.post('/validate-api-key', {}, { headers: { 'X-API-Key': apiKey } }), |
Copilot uses AI. Check for mistakes.
}, | ||
}, | ||
"limit": map[string]interface{}{ | ||
"requests": rateLimiting.RequestsPerMinute, |
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 rate-limiting policy only enforces requests per minute; the RequestsPerHour
and BurstLimit
fields are not applied. You should add additional rules for per-hour limits and burst handling.
Copilot uses AI. Check for mistakes.
if rateLimiting.TokensPerHour > 0 { | ||
rules := policy["spec"].(map[string]interface{})["rateLimit"].(map[string]interface{})["global"].(map[string]interface{})["rules"].([]interface{}) | ||
|
||
// Add token-based rate limiting |
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 section handles token limits but doesn’t enforce the configured RequestsPerHour
or BurstLimit
. Consider appending an additional rule for those values.
if rateLimiting.TokensPerHour > 0 { | |
rules := policy["spec"].(map[string]interface{})["rateLimit"].(map[string]interface{})["global"].(map[string]interface{})["rules"].([]interface{}) | |
// Add token-based rate limiting | |
rules := policy["spec"].(map[string]interface{})["rateLimit"].(map[string]interface{})["global"].(map[string]interface{})["rules"].([]interface{}) | |
// Add token-based rate limiting | |
if rateLimiting.TokensPerHour > 0 { |
Copilot uses AI. Check for mistakes.
color: #2563eb; | ||
} | ||
|
||
.form-section { |
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.
[nitpick] The .form-section
selector is defined twice (around line 101 and here). Consolidate duplicate rules to reduce CSS bloat and avoid conflicting styles.
Copilot uses AI. Check for mistakes.
}; | ||
|
||
const renderStepIndicator = () => ( | ||
<div style={{ |
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.
[nitpick] There are large inline style blocks for the step indicator; consider moving these into CSS classes to improve readability and reuse.
Copilot uses AI. Check for mistakes.
This pull request introduces a comprehensive model publishing workflow for external access, along with updates to authentication, infrastructure, and documentation. The changes include defining requirements, implementing the publishing workflow, enhancing authentication mechanisms, and removing deprecated configurations.
Model Publishing Workflow:
.kiro/specs/model-publishing-workflow/requirements.md
: Added a detailed requirements document outlining user stories, acceptance criteria, and features for publishing inference models securely, including tenant-specific access controls, API key generation, and rate limiting..kiro/specs/model-publishing-workflow/tasks.md
: Defined an extensive implementation plan with tasks for setting up publishing infrastructure, extending Kubernetes client operations, creating API endpoints, and adding monitoring and UI components.Authentication Enhancements:
management/auth.go
: IntroducedEnhancedAuthMiddleware
to validate both JWT tokens and API keys, added API key validation logic, and implemented metadata search across namespaces for tenant-specific access.management/main.go
: Updated service initialization to includePublishingService
and integrated it with the HTTP server setup.Infrastructure Updates:
configs/management/management.yaml.bak
: Removed deprecated configurations for themanagement-service
, including service account, RBAC roles, and deployment specifications.Documentation Improvements:
docs/getting-started.md
: Updated the repository URL in the cloning instructions and added minor formatting improvements to the setup script description. [1] [2]Minor Code Changes:
management/admin.go
: Added thelog
package import to improve logging capabilities.configs/kserve/models/huggingface-t5.yaml
: Removed the--backend=huggingface
argument from the model configuration.