-
Notifications
You must be signed in to change notification settings - Fork 0
feat: 添加凭证支持到模型服务,增强 API 密钥管理 #5
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
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 fixes a bug where model services were not correctly retrieving API keys when credentials were configured. The fix adds logic to fetch the API key from the associated credential when it's not directly provided in the provider settings.
- Added credential-based API key retrieval in
model_info()method - Added comprehensive test coverage for model services using credentials (both sync and async)
- Removed unused
ClientErrorimport from test files
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| agentrun/model/model_service.py | Added logic to retrieve API key from credential when not set in provider settings |
| agentrun/model/__model_service_async_template.py | Added same credential-based API key retrieval logic for async template |
| tests/e2e/test_model.py | Added sync and async test cases for model services with credentials; removed unused import |
| tests/e2e/__test_model_async_template.py | Added async test case for model services with credentials; removed unused import |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cr = Credential.create( | ||
| CredentialCreateInput( | ||
| credential_name=f"{model_service_name}-credential", | ||
| enabled=True, | ||
| credential_config=CredentialConfig.outbound_llm_api_key( | ||
| api_key=api_key, | ||
| provider="openai", | ||
| ), | ||
| ) | ||
| ) |
Copilot
AI
Dec 2, 2025
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.
In an async test method, Credential.create() should be replaced with await Credential.create_async() to maintain consistency with the async pattern used elsewhere in the test (e.g., await ModelService.create_async(), await ms.delete_async(), await cr.delete_async()). Using synchronous calls in async methods can lead to blocking behavior and defeats the purpose of async testing.
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.
@copilot open a new pull request to apply changes based on this feedback
2e9e2cb to
765650b
Compare
Change-Id: Ib6831f5a3978b3a47843c3c9f22c1b63c2255c32 Signed-off-by: OhYee <oyohyee@oyohyee.com>
be7573c to
4f3b53d
Compare
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
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Change-Id: Idf3ecad27a33902fc017c9392e314b252944c7ab
Fix bugs
Bug detail
Pull request tasks
Update docs
Reason for update
Pull request tasks
Add contributor
Contributed content
Content detail
Others
Reason for update