-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat(dashscope): support to load headers from the environment #1096
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
base: main
Are you sure you want to change the base?
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 pull request adds support for loading HTTP headers from the DASHSCOPE_API_HEADERS environment variable for DashScope API calls. This allows users to configure custom headers without modifying code.
Key Changes
- Added environment variable loading for
DASHSCOPE_API_HEADERSin theDashScopeChatModelinitialization - Headers from the environment variable are parsed as JSON and merged with explicitly provided headers in
generate_kwargs - Error handling ensures invalid JSON triggers a warning rather than failing initialization
|
/gemini 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.
Code Review
This pull request adds a useful feature to load DashScope headers from an environment variable. The implementation is mostly correct, and the identified potential bug and suggested refactoring are valid and improve robustness. The original comment aligns with best practices and does not contradict any provided rules, so it has been kept as is.
| # Load headers from environment variable if exists | ||
| headers = os.getenv("DASHSCOPE_API_HEADERS") | ||
| if headers: | ||
| try: | ||
| headers = json.loads(str(headers)) | ||
| if not isinstance(headers, dict): | ||
| raise json.JSONDecodeError("", "", 0) | ||
|
|
||
| if self.generate_kwargs.get("headers"): | ||
| headers.update(self.generate_kwargs["headers"]) | ||
|
|
||
| self.generate_kwargs["headers"] = headers | ||
|
|
||
| except json.JSONDecodeError: | ||
| logger.warning( | ||
| "Failed to parse DASHSCOPE_API_HEADERS environment " | ||
| "variable as JSON. It should be a JSON object.", | ||
| ) |
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 current implementation for loading headers from the environment can be improved for robustness and clarity.
- Potential Bug: The line
headers.update(self.generate_kwargs["headers"])assumesself.generate_kwargs["headers"]is a dictionary. If it's another type (e.g., a list or string, which is possible given theJSONSerializableObjecttype hint), this will raise an unhandled exception, crashing the application during model initialization. - Unconventional Error Handling: Manually raising
json.JSONDecodeErrorfor a type check is not standard practice. It's better to handle invalid types explicitly, which also allows for more specific error messages. - Redundancy: The
str(headers)cast is unnecessary asos.getenvalready returns a string.
I suggest refactoring this block to address these issues. The following code is more robust, handles potential type errors gracefully, and provides clearer log messages.
# Load headers from environment variable if exists
headers_env_str = os.getenv("DASHSCOPE_API_HEADERS")
if headers_env_str:
try:
headers = json.loads(headers_env_str)
if not isinstance(headers, dict):
raise TypeError(
"Value from DASHSCOPE_API_HEADERS is not a dict",
)
kw_headers = self.generate_kwargs.get("headers")
if isinstance(kw_headers, dict):
headers.update(kw_headers)
elif kw_headers is not None:
logger.warning(
"The 'headers' in generate_kwargs is not a "
"dictionary and will be ignored.",
)
self.generate_kwargs["headers"] = headers
except (json.JSONDecodeError, TypeError) as e:
logger.warning(
"Failed to load headers from DASHSCOPE_API_HEADERS "
"environment variable: %s. It should be a JSON object.",
e,
)
AgentScope Version
[The version of AgentScope you are working on, e.g.
import agentscope; print(agentscope.__version__)]Description
[Please describe the background, purpose, changes made, and how to test this PR]
Checklist
Please check the following items before code is ready to be reviewed.
pre-commit run --all-filescommand