Move start session to thread #27
Conversation
Co-authored-by: Howard Gil <[email protected]>
WalkthroughThis update primarily focuses on enhancing the session management within the Changes
Files selected (28)
Files ignored (20)
InstructionsEmoji Descriptions:
Interact with the Bot:
Available Commands:
Tips for Using @Entelligence.AI Effectively:
Need More Help?
|
agentops/session.py
Outdated
| config: Configuration, | ||
| tags: Optional[List[str]] = None, | ||
| host_env: Optional[dict] = None, | ||
| callback: Optional[Callable[["Session"], None]] = None, | ||
| ): | ||
| self.end_timestamp = None | ||
| self.end_state: Optional[str] = None |
There was a problem hiding this comment.
ℹ️ Logic Error
Update Callback Functions to Match New Signature
The change in the callback parameter type from Callable[[bool], None] to Callable[["Session"], None] in the __init__ method could lead to unexpected behavior if the callback functions are not updated accordingly. Ensure that all callback functions are compatible with the new signature to prevent runtime errors.
Commitable Code Suggestion:
| config: Configuration, | |
| tags: Optional[List[str]] = None, | |
| host_env: Optional[dict] = None, | |
| callback: Optional[Callable[["Session"], None]] = None, | |
| ): | |
| self.end_timestamp = None | |
| self.end_state: Optional[str] = None | |
| def __init__( | |
| config: Configuration, | |
| tags: Optional[List[str]] = None, | |
| host_env: Optional[dict] = None, | |
| callback: Optional[Callable[["Session"], None]] = None, | |
| ): | |
| self.end_timestamp = None | |
| self.end_state: Optional[str] = None |
There was a problem hiding this comment.
@Entelligence.AI what is a callback function?
There was a problem hiding this comment.
A callback function is a function that is passed into another function as an argument and is intended to be executed after a certain event or operation is completed. In programming, callbacks are often used to handle asynchronous operations or to customize the behavior of a function by allowing the caller to provide additional logic to be executed at a specific point in the process.
There was a problem hiding this comment.
@Entelligence.AI what is a callback function?
A callback function is a function that is passed as an argument to another function and is executed after a certain event occurs or to handle asynchronous tasks.
There was a problem hiding this comment.
@Entelligence.AI /updateGuideline do not comment on callbacks
There was a problem hiding this comment.
Guideline updated successfully: user_guidelines:do not comment on callbacks
There was a problem hiding this comment.
@Entelligence.AI /updateGuideline do not comment on callbacks
Guideline updated successfully: do not comment on callbacks
There was a problem hiding this comment.
@Entelligence.AI /updateCommit
There was a problem hiding this comment.
I have successfully committed the changes!
There was a problem hiding this comment.
@Entelligence.AI /updateCommit
Error committing file content: 409 - {'message': 'agentops/session.py does not match ab3a833fd3f46b69c001efc151006f3564e6c553', 'documentation_url': 'https://docs.github.com/rest/repos/contents#create-or-update-file-contents', 'status': '409'}
| self._sessions.remove(session) | ||
|
|
||
| session = Session( | ||
| session_id=session_id, | ||
| tags=list(session_tags), | ||
| host_env=get_host_env(self._config.env_data_opt_out), | ||
| config=self._config, | ||
| callback=_start_session_callback, |
There was a problem hiding this comment.
ℹ️ Logic Error
Verify Session State Before Removal
The logic for handling sessions has been altered to remove a session if it is not running. Ensure that this change does not inadvertently remove sessions that are intended to be retained. Consider adding a condition to verify the session's state before removal.
+ elif not session.is_running:
+ self._sessions.remove(session)Commitable Code Suggestion:
| self._sessions.remove(session) | |
| session = Session( | |
| session_id=session_id, | |
| tags=list(session_tags), | |
| host_env=get_host_env(self._config.env_data_opt_out), | |
| config=self._config, | |
| callback=_start_session_callback, | |
| elif not session.is_running: | |
| self._sessions.remove(session) |
There was a problem hiding this comment.
@Entelligence.AI /updateCommit
There was a problem hiding this comment.
I have successfully committed the changes!
There was a problem hiding this comment.
@Entelligence.AI /updateCommit
Error committing file content: 409 - {'message': 'agentops/client.py does not match 0942f1e25f7d0290d2face529c8fc96cd660ee44', 'documentation_url': 'https://docs.github.com/rest/repos/contents#create-or-update-file-contents', 'status': '409'}
| self.stop_flag = threading.Event() | ||
| self.thread = threading.Thread(target=self._run) | ||
| self.thread = threading.Thread(target=self._run, args=(callback,)) | ||
| self.thread.daemon = True | ||
| self.thread.start() | ||
|
|
||
| self.is_running = self._start_session() | ||
| if self.is_running == False: | ||
| self.stop_flag.set() | ||
| self.thread.join(timeout=1) | ||
|
|
||
| def set_video(self, video: str) -> None: |
There was a problem hiding this comment.
ℹ️ Logic Error
Potential Delay in Session Start
The removal of the session start logic from the __init__ method to the _run method could lead to a situation where the session is not started immediately upon object creation. This might cause unexpected behavior if other methods are called before _run is executed.
Commitable Code Suggestion:
| self.stop_flag = threading.Event() | |
| self.thread = threading.Thread(target=self._run) | |
| self.thread = threading.Thread(target=self._run, args=(callback,)) | |
| self.thread.daemon = True | |
| self.thread.start() | |
| self.is_running = self._start_session() | |
| if self.is_running == False: | |
| self.stop_flag.set() | |
| self.thread.join(timeout=1) | |
| def set_video(self, video: str) -> None: | |
| def __init__(self): | |
| self.thread.daemon = True | |
| self.thread.start() | |
| self.is_running = self._start_session() | |
| if not self.is_running: | |
| self.stop_flag.set() | |
| self.thread.join(timeout=1) |
… __init__ method to match the new signature.
Files selected (1)
Files ignored (0)InstructionsEmoji Descriptions:
Interact with the Bot:
Available Commands:
Tips for Using @Entelligence.AI Effectively:
Need More Help?
|
| callback: Optional[Callable[["Session"], None]] = None, | ||
| ): | ||
| self.end_timestamp = None | ||
| self.end_state: Optional[str] = None | ||
| self.session_id = session_id | ||
| self.end_state: Optional[str] = None self.session_id = session_id | ||
| self.init_timestamp = get_ISO_time() | ||
| self.tags: List[str] = tags or [] | ||
| self.video: Optional[str] = None |
There was a problem hiding this comment.
ℹ️ Logic Error
Fix Syntax Error in Attribute Initialization
The initialization of session_id is missing a line break, which could lead to a syntax error. Ensure each attribute is initialized on a separate line to maintain code readability and prevent potential issues.
def __init__(
callback: Optional[Callable[["Session"], None]] = None,
):
self.end_timestamp = None
self.end_state: Optional[str] = None
+ self.session_id = session_id
self.init_timestamp = get_ISO_time()
self.tags: List[str] = tags or []
self.video: Optional[str] = NoneCommitable Code Suggestion:
| callback: Optional[Callable[["Session"], None]] = None, | |
| ): | |
| self.end_timestamp = None | |
| self.end_state: Optional[str] = None | |
| self.session_id = session_id | |
| self.end_state: Optional[str] = None self.session_id = session_id | |
| self.init_timestamp = get_ISO_time() | |
| self.tags: List[str] = tags or [] | |
| self.video: Optional[str] = None | |
| def __init__( | |
| callback: Optional[Callable[["Session"], None]] = None, | |
| ): | |
| self.end_timestamp = None | |
| self.end_state: Optional[str] = None | |
| self.session_id = session_id | |
| self.init_timestamp = get_ISO_time() | |
| self.tags: List[str] = tags or [] | |
| self.video: Optional[str] = None |
![kaizen-bot[bot]](https://avatars.githubusercontent.com/in/480502?s=60&v=4){kind=small}
agentops/client.py
Outdated
| def _start_session_callback(session: Session): | ||
| if session.is_running: | ||
| if len(self._pre_init_queue["agents"]) > 0: | ||
| for agent_args in self._pre_init_queue["agents"]: | ||
| session.create_agent( | ||
| name=agent_args["name"], agent_id=agent_args["agent_id"] | ||
| ) | ||
| self._pre_init_queue["agents"] = [] | ||
|
|
||
| logger.info( | ||
| colored( | ||
| f"\x1b[34mSession Replay: https://app.agentops.ai/drilldown?session_id={session.session_id}\x1b[0m", | ||
| "blue", | ||
| ) | ||
| ) | ||
| else: | ||
| self._sessions.remove(session) |
There was a problem hiding this comment.
Comment: Optimize session start callback handling
Solution: Modify the _start_session_callback function to only create agents when the session is actually running, and handle the case where the session is not running properly.
!! Make sure the following suggestion is correct before committing it !!
| def _start_session_callback(session: Session): | |
| if session.is_running: | |
| if len(self._pre_init_queue["agents"]) > 0: | |
| for agent_args in self._pre_init_queue["agents"]: | |
| session.create_agent( | |
| name=agent_args["name"], agent_id=agent_args["agent_id"] | |
| ) | |
| self._pre_init_queue["agents"] = [] | |
| logger.info( | |
| colored( | |
| f"\x1b[34mSession Replay: https://app.agentops.ai/drilldown?session_id={session.session_id}\x1b[0m", | |
| "blue", | |
| ) | |
| ) | |
| else: | |
| self._sessions.remove(session) | |
| ['if session.is_running:', ' while self._pre_init_queue["agents"]:', ' agent_args = self._pre_init_queue["agents"].pop(0)', ' session.create_agent(', ' name=agent_args["name"], agent_id=agent_args["agent_id"]', ' )', 'else:', ' self._pre_init_queue["agents"].clear()', ' self._sessions.remove(session)'] |
…ntended session deletions.
Files selected (1)
Files ignored (0)InstructionsEmoji Descriptions:
Interact with the Bot:
Available Commands:
Tips for Using @Entelligence.AI Effectively:
Need More Help?
|
🔍 Code Review Summary❗ Attention Required: This push has potential issues. 🚨 Overview
🚨 Critical Issuessecurity (2 issues)1. Potential exposure of sensitive information in logs.📁 File: agentops/session.py 💡 Solution: Current Code: logger.error(f"Could not start session - server could not authenticate your API Key")Suggested Code: logger.error("Could not start session - server could not authenticate your API Key")2. Unnecessary sleep calls in tests can lead to longer test execution times.📁 File: tests/test_pre_init.py 💡 Solution: Current Code: time.sleep(1)Suggested Code: # Mock time.sleep or use a testing framework that supports time manipulation.
Useful Commands
|
| "blue", | ||
| ) | ||
| ) | ||
| elif not session.is_running: | ||
| self._sessions.remove(session) | ||
|
|
||
| session = Session( |
There was a problem hiding this comment.
ℹ️ Logic Error
Ensure Sessions are Only Removed if Not Running
The change in the condition from else to elif not session.is_running ensures that only non-running sessions are removed. This is a critical fix to prevent the removal of active sessions, which could lead to unexpected behavior or errors in session management.
Commitable Code Suggestion:
| "blue", | |
| ) | |
| ) | |
| elif not session.is_running: | |
| self._sessions.remove(session) | |
| session = Session( | |
| elif not session.is_running: | |
| self._sessions.remove(session) |
| callback=_start_session_callback, | ||
| ) | ||
|
|
||
| if not session.is_running: | ||
| return logger.error("Failed to start session") | ||
| if not session.is_running: return logger.error("Failed to start session") | ||
|
|
||
| if self._pre_init_queue["agents"] and len(self._pre_init_queue["agents"]) > 0: | ||
| for agent_args in self._pre_init_queue["agents"]: |
There was a problem hiding this comment.
💻 Code Readability
Consolidate Error Logging and Return Statement
The consolidation of the return statement with the error logging improves code readability. However, ensure that the logging message is clear and provides enough context for debugging purposes.
+ if not session.is_running:
+ return logger.error("Failed to start session: Session is not running")Commitable Code Suggestion:
| callback=_start_session_callback, | |
| ) | |
| if not session.is_running: | |
| return logger.error("Failed to start session") | |
| if not session.is_running: return logger.error("Failed to start session") | |
| if self._pre_init_queue["agents"] and len(self._pre_init_queue["agents"]) > 0: | |
| for agent_args in self._pre_init_queue["agents"]: | |
| if not session.is_running: | |
| return logger.error("Failed to start session: Session is not running") |
| f"Could not start session - server could not authenticate your API Key" | ||
| ) |
There was a problem hiding this comment.
Comment: Potential exposure of sensitive information in logs.
Solution: Ensure that sensitive information is masked or omitted from logs.
!! Make sure the following suggestion is correct before committing it !!
| f"Could not start session - server could not authenticate your API Key" | |
| ) | |
| logger.error("Could not start session - server could not authenticate your API Key") |
| @@ -48,10 +48,20 @@ def test_track_agent(self, mock_req): | |||
| assert len(mock_req.request_history) == 0 | |||
|
|
|||
| agentops.init(api_key=self.api_key) | |||
| time.sleep(1) | |||
There was a problem hiding this comment.
Comment: Unnecessary sleep calls in tests can lead to longer test execution times.
Solution: Use mocking to simulate time delays instead of using sleep.
!! Make sure the following suggestion is correct before committing it !!
| time.sleep(1) | |
| # Mock time.sleep or use a testing framework that supports time manipulation. |
Enhance AgentOps Initialization and Session Management
Improve the initialization process and session management in the AgentOps client.
_initialize_autogen_loggermethod for better logging during autogen initialization.Sessionclass to handle session state changes.These changes enhance logging, improve session handling, and increase test reliability, leading to a more robust AgentOps client.
Original Description
# Improve Autogen Logger Integration**
Enhance the integration of the Autogen logger with the AgentOps client.
agentopsmodule._initialize_autogen_logger()in theClientclass to handle the Autogen logger setup.**
This change will improve the seamless integration of the Autogen logger, providing better logging and debugging capabilities for the AgentOps client.
Original Description
# 🔍 Review Summary **Purpose**: Enhance session management within the `agentops` module to improve asynchronous session handling and reliability.Key Changes:
start_sessionmethod non-blocking and introduced a callback mechanism for asynchronous session handling.0.3.12to0.3.14and unpinned dependencies forrequestsandPyYAMLinpyproject.toml.Impact:
Improves the user experience by allowing non-blocking session starts and ensuring more reliable asynchronous session handling. The version update and dependency unpinning provide flexibility for future versions, and the test fixture adjustments enhance the overall testing process.
Original Description
No existing description found