use proper state loading condition#460
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors AgentState.load() to centralize the “should we read from storage?” decision into a new _should_load() helper, with the goal of preventing unnecessary storage reads and aligning the caching pattern with the .NET SDK (per issue #440).
Changes:
- Replaced the inline
load()guard (force or not self._cached_state) with a new_should_load()helper. - Updated the load condition to consult the turn-scoped cached state (
turn_context.turn_state) and to additionally consider “empty cached state”.
💡 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 41 out of 45 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
dev/hosting_dialogs/memory/scopes/test_memory_scopes.py:216
- Calling ConversationState.save() here is a no-op because the state has not been loaded/initialized yet (AgentState._cached_state is still None). If the intent is to ensure the ConversationState cache exists for this turn, call load() instead.
dev/hosting_dialogs/memory/scopes/test_memory_scopes.py:288 - Calling ConversationState.save() here is a no-op because the state has not been loaded/initialized yet (AgentState._cached_state is still None). If the intent is to ensure the ConversationState cache exists for this turn, call load() instead.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/state/agent_state.py:269
- delete_value() checks
self._cached_state.state.get(property_name)before deleting. This fails to delete keys whose stored value is falsy (e.g., False/0/empty string), even though the property exists. Use membership (property_name in ...) instead of truthiness.
if not property_name:
raise TypeError(
"AgentState.delete_value(): property_name cannot be None or empty."
)
if self._cached_state.state.get(property_name):
del self._cached_state.state[property_name]
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/state/agent_state.py:269
- delete_value() uses
self._cached_state.state.get(property_name)as the existence check. That fails to delete entries whose stored value is falsey (e.g., 0, "", False), leaving the key in state unexpectedly.
"AgentState.delete_value(): property_name cannot be None or empty."
)
if self._cached_state.state.get(property_name):
del self._cached_state.state[property_name]
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
libraries/microsoft-agents-hosting-dialogs/microsoft_agents/hosting/dialogs/dialog_manager.py:93
- on_turn() now skips resolving ConversationState from context.turn_state, but still unconditionally uses self.conversation_state immediately after. If conversation_state was not configured, this will raise an AttributeError at create_property(). Fail fast with a clear error (or restore a resolution mechanism) before using conversation_state.
if self.conversation_state is not None:
await self.conversation_state.load(context, False)
if self.user_state is not None:
await self.user_state.load(context, False)
# Create property accessors
last_access_property = self.conversation_state.create_property(self.last_access)
last_access: datetime = cast(
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
… outputs consistent with what the program expects
bb678f8 to
8f91a50
Compare
fixes #440 using same design pattern as Agents-for-net
This pull request refactors the state loading logic in the agent state management code to improve cache handling and test clarity. The main changes introduce a helper method to determine when state should be loaded from storage, and update tests to use the new cache access pattern.
Agent state loading improvements:
_should_loadmethod to encapsulate the logic for when the agent state should be loaded from storage, improving readability and maintainability. Theloadmethod now uses this helper instead of directly checking the cache.Testing enhancements:
get_cached_statemethod after update, ensuring tests reflect the current cache access pattern.Also removes a module which misuses the state loading/saving design pattern, and tests which should have already been deleted