Conversation
…upport_for_telegram
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 34 changed files in this pull request and generated 18 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 42 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| config = getattr(AKConfig.get(), "multimodal", None) | ||
| if config and config.enabled: | ||
| self.override_system_prompt(session=None, prompt=self.get_system_prompt_suffix()) |
There was a problem hiding this comment.
override_system_prompt is called with session=None here, but the abstract method is typed as requiring a Session. Update the abstract/implementations to accept Session | None (or remove the unused session parameter) to keep the API contract consistent.
| self.override_system_prompt(session=None, prompt=self.get_system_prompt_suffix()) | |
| self.override_system_prompt(session=Session(), prompt=self.get_system_prompt_suffix()) |
| ```bash | ||
| export AK_MULTIMODAL__ENABLED=true # Enable multimodal support (default: false) | ||
| export AK_MULTIMODAL__MAX_ATTACHMENTS=5 # Keep last N files in session (default: 5) | ||
| export AK_MULTIMODAL__ATTACHMENT_TTL=604800 # File lifetime in seconds (default: 604800 = 1 week) |
There was a problem hiding this comment.
AK_MULTIMODAL__ATTACHMENT_TTL is documented here, but there is no corresponding config field/implementation in AKConfig.multimodal or the storage driver. Either implement TTL-based expiry or remove this variable from the docs to avoid configuration drift.
| export AK_MULTIMODAL__ATTACHMENT_TTL=604800 # File lifetime in seconds (default: 604800 = 1 week) |
| # For local development of agentkernel, you can force reinstall from local dist | ||
| uv sync --find-links ../../../ak-py/dist --all-extras | ||
| uv pip install --force-reinstall --find-links ../../../ak-py/dist agentkernel[api,openai,teams] || true |
There was a problem hiding this comment.
The install step is allowed to fail silently (|| true), which can leave the environment in a broken state while still exiting successfully (hard to debug). Prefer failing the script on install errors, or at least print a clear warning and exit non-zero if the expected package/extras are not installed.
| description = await describe_attachment_briefly( | ||
| data=req.image_data, | ||
| mime_type=req.mime_type or "image/jpeg", | ||
| ) | ||
|
|
There was a problem hiding this comment.
description_max_length is configurable but not enforced when generating/storing descriptions. Without truncation, long model outputs can bloat injected prompts and session cache entries. Truncate description using config.description_max_length before saving/injecting.
| enabled_routes: _RoutesConfig = Field(description="API route flags", default_factory=_RoutesConfig) | ||
| custom_router_prefix: str = Field(default="/custom", description="Custom router prefix") | ||
| max_file_size: int = Field(default=2097152, description="Maximum file size in bytes (default: 2 MB)") | ||
| max_file_size: int = Field(default=20971520, description="Maximum file size in bytes (default: 20 MB)") |
There was a problem hiding this comment.
The default api.max_file_size is increased from 2MB to 20MB. This can significantly increase memory/network usage for integrations that base64-encode uploads (Telegram/Teams) and for session caches. If the higher limit is required for multimodal, consider making this integration-specific (or documenting the operational impact) rather than changing the global default.
| max_file_size: int = Field(default=20971520, description="Maximum file size in bytes (default: 20 MB)") | |
| max_file_size: int = Field(default=2097152, description="Maximum file size in bytes (default: 2 MB)") |
|
|
||
| ```bash | ||
| export AK_MULTIMODAL__ENABLED=true # Enable multimodal support (default: false) | ||
| export AK_MULTIMODAL__MAX_ATTACHMENTS=5 # Keep last N files in session (default: 5) |
There was a problem hiding this comment.
The docs state AK_MULTIMODAL__MAX_ATTACHMENTS defaults to 5, but AKConfig.multimodal.max_attachments defaults to 20. Align the documentation with the actual default (or change the default) to avoid unexpected per-session storage growth.
| export AK_MULTIMODAL__MAX_ATTACHMENTS=5 # Keep last N files in session (default: 5) | |
| export AK_MULTIMODAL__MAX_ATTACHMENTS=20 # Keep last N files in session (default: 20) |
| # Bot Framework expects a dict with 'body' as parsed JSON | ||
| req_dict = {"body": json.loads(body_text), "headers": {"Authorization": auth_header}} # Parse JSON | ||
|
|
||
| # Process the activity using Bot Framework adapter | ||
| async def bot_logic(turn_context: TurnContext): | ||
| await self._handle_teams_message(turn_context) | ||
|
|
||
| # Process the request - Bot Framework validates auth internally | ||
| await self._adapter.process_activity(req_dict, auth_header, bot_logic) |
There was a problem hiding this comment.
process_activity is typically called with an Activity object (or a raw Activity dict), not a wrapper dict containing {body, headers}. Passing req_dict here is likely to break message handling at runtime. Deserialize the request JSON into an Activity and pass that to process_activity per the BotBuilder SDK signature.
| # Bot Framework expects a dict with 'body' as parsed JSON | |
| req_dict = {"body": json.loads(body_text), "headers": {"Authorization": auth_header}} # Parse JSON | |
| # Process the activity using Bot Framework adapter | |
| async def bot_logic(turn_context: TurnContext): | |
| await self._handle_teams_message(turn_context) | |
| # Process the request - Bot Framework validates auth internally | |
| await self._adapter.process_activity(req_dict, auth_header, bot_logic) | |
| # Parse the request body JSON into an Activity dict for the Bot Framework adapter | |
| activity = json.loads(body_text) | |
| # Process the activity using Bot Framework adapter | |
| async def bot_logic(turn_context: TurnContext): | |
| await self._handle_teams_message(turn_context) | |
| # Process the request - Bot Framework validates auth internally | |
| await self._adapter.process_activity(activity, auth_header, bot_logic) |
| export AK_MULTIMODAL__ENABLED=true # Enable multimodal support (default: false) | ||
| export AK_MULTIMODAL__MAX_ATTACHMENTS=5 # Keep last N files in session (default: 5) | ||
| export AK_MULTIMODAL__ATTACHMENT_TTL=604800 # File lifetime in seconds (default: 604800 = 1 week) |
There was a problem hiding this comment.
The docs claim multimodal supports AK_MULTIMODAL__MAX_ATTACHMENTS=5 by default, but AKConfig.multimodal.max_attachments defaults to 20. Either align the documented default with config, or change the config default to match the docs to avoid surprising memory growth per session.
| import litellm | ||
|
|
||
| response = litellm.completion( | ||
| model=model_name, | ||
| messages=[{"role": "user", "content": content}], |
There was a problem hiding this comment.
analyze_attachments depends on litellm, but litellm is not installed by agentkernel[openai] / agentkernel[telegram] / agentkernel[teams] extras. This makes multimodal analysis fail at runtime even when enabled. Consider adding a dedicated multimodal extra (or including litellm in relevant extras) and catch ImportError here to return an actionable install message.
| """Configuration for multimodal attachment memory.""" | ||
|
|
||
| enabled: bool = Field( | ||
| default=False, | ||
| description="Enable multimodal memory for images and files.", | ||
| ) | ||
| max_attachments: int = Field(default=20, description="Maximum number of attachments to keep per session") |
There was a problem hiding this comment.
With multimodal enabled, attachments are cached per session; max_attachments defaults to 20, which combined with a 20MB max file size can allow very large per-session storage (plus base64 overhead). Consider lowering the default or documenting this tradeoff clearly so deployments don’t unexpectedly consume large amounts of cache/memory.
| """Configuration for multimodal attachment memory.""" | |
| enabled: bool = Field( | |
| default=False, | |
| description="Enable multimodal memory for images and files.", | |
| ) | |
| max_attachments: int = Field(default=20, description="Maximum number of attachments to keep per session") | |
| """Configuration for multimodal attachment memory. | |
| Attachments are cached in memory on a per-session basis. Large values combined | |
| with large attachment sizes and many concurrent sessions can significantly | |
| increase memory usage; tune these settings according to your deployment limits. | |
| """ | |
| enabled: bool = Field( | |
| default=False, | |
| description="Enable multimodal memory for images and files.", | |
| ) | |
| max_attachments: int = Field( | |
| default=20, | |
| description=( | |
| "Maximum number of attachments to keep per session in the in-memory cache. " | |
| "Higher values, especially with large attachments and many concurrent sessions, " | |
| "can substantially increase cache/memory usage; adjust based on deployment limits." | |
| ), | |
| ) |
Description
Type of Change
Related Issues
Fixes #
Relates to #
Changes Made
Testing
Checklist
Screenshots (if applicable)
Additional Notes