feat: add async context manager support for CopilotClient and Copilot…#475
feat: add async context manager support for CopilotClient and Copilot…#475Sumanth007 wants to merge 3 commits intogithub:mainfrom
Conversation
…Session with automatic resource cleanup
There was a problem hiding this comment.
Pull request overview
Adds async context manager (async with) support to the Python SDK’s CopilotClient and CopilotSession so callers can get automatic startup/teardown, and updates Python docs + tests to encourage/verify the pattern.
Changes:
- Implemented
__aenter__/__aexit__onCopilotClientto auto-start()and cleanup viastop(). - Implemented
__aenter__/__aexit__onCopilotSessionto auto-cleanup viadestroy(). - Added unit/E2E tests and updated
python/README.mdto recommendasync withusage.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| python/copilot/client.py | Adds async context manager methods and cleanup logging for the client lifecycle. |
| python/copilot/session.py | Adds async context manager methods for session lifecycle cleanup. |
| python/test_client.py | Adds unit tests for context manager return values / exception propagation behavior. |
| python/e2e/test_context_managers.py | Adds E2E coverage for client/session context manager behavior and cleanup semantics. |
| python/README.md | Documents and recommends async with usage patterns for safer resource cleanup. |
… for CopilotClient and CopilotSession
| pytestmark = pytest.mark.asyncio(loop_scope="module") | ||
|
|
||
|
|
There was a problem hiding this comment.
The E2E tests require snapshot files in the test/snapshots/context_managers/ directory to function properly. The test harness expects YAML snapshot files for each test to replay CAPI responses. The snapshot directory test/snapshots/context_managers/ and corresponding YAML files (e.g., should_auto_start_and_cleanup_with_context_manager.yaml, should_create_session_in_context.yaml, etc.) are missing.
The tests will fail without these snapshot files because the replaying proxy uses them to mock API responses. You'll need to either:
- Create the snapshot directory and YAML files with appropriate test data
- Generate the snapshots by running the tests in record mode first
- Copy and adapt existing snapshot files from similar tests
| pytestmark = pytest.mark.asyncio(loop_scope="module") | |
| SNAPSHOT_DIR = os.path.join( | |
| os.path.dirname(os.path.dirname(__file__)), | |
| "test", | |
| "snapshots", | |
| "context_managers", | |
| ) | |
| pytestmark = [ | |
| pytest.mark.asyncio(loop_scope="module"), | |
| pytest.mark.skipif( | |
| not os.path.isdir(SNAPSHOT_DIR), | |
| reason="context_managers snapshots are missing; run E2E in record mode to generate them", | |
| ), | |
| ] |
brettcannon
left a comment
brettcannon
left a comment
There was a problem hiding this comment.
This PR opens the question of whether embedding logging support is desired, and if so how to do that.
It also opens the question of whether to suppress exceptions when stopping a session or destroying a client. Typically you only suppress stuff that you simply don't care about or provide a way to gain access to the suppressed details after exiting the context manager. But usually you call nearly infallible code that you don't worry about the failures. There's a couple of ways to support this (e.g. attach failures to the object returned by __aenter__, return a tuple where the second object is a list that will contain the failure objects, etc.). But since you can always put a with inside a try, I don't think hiding all exceptions is the right approach and the question is how to handle destroy() returning anything.
| # Log any session destruction errors | ||
| if stop_errors: | ||
| for error in stop_errors: | ||
| logging.warning("Error during session cleanup in CopilotClient: %s", error) |
There was a problem hiding this comment.
I'm a bit concerned about this as not everyone wants 'logging' in their code as they may use a different logger. Plus this is too broad regardless as there isn't a way to filter out the logging for the library (i.e. this call uses the root logger).
I think more thought has to go into whether there's a desire for logging, and if there is then how to handle it.
| except Exception: | ||
| # Log the error but don't raise - we want cleanup to always complete | ||
| logging.warning("Error during CopilotClient cleanup", exc_info=True) | ||
| return False |
There was a problem hiding this comment.
This is incorrect if you're trying to suppress the exception; you would want to return True in that instance.
| stop_errors = await self.stop() | ||
| # Log any session destruction errors | ||
| if stop_errors: | ||
| for error in stop_errors: |
There was a problem hiding this comment.
If StopError was an exception then you would want to use ExceptionGroup, but since that's a Python 3.11 thing and this library supports 3.9 it would need a shim for 3.9 and 3.10.
| except Exception: | ||
| # Log the error but don't raise - we want cleanup to always complete | ||
| logging.warning("Error during CopilotSession cleanup", exc_info=True) | ||
| return False |
There was a problem hiding this comment.
I don't think the exception that was raised should be suppressed without at least a configuration option.
2 participants
Session with automatic resource cleanup #341
This pull request introduces async context manager support for both
CopilotClientandCopilotSession, enabling automatic resource cleanup and following Python best practices for resource management. The documentation is updated to recommend this pattern, and comprehensive tests ensure correct behavior and error handling. These changes make it easier and safer to use the SDK, especially in batch or evaluation scenarios.Async context manager support and resource management:
__aenter__and__aexit__) toCopilotClientandCopilotSessionfor automatic startup, cleanup, and error handling, ensuring resources are always released—even if exceptions occur. [1] [2]python/README.mdto document and recommend the context manager usage pattern, including example code and benefits, and highlighted this feature in the capabilities list. [1] [2] [3]Testing and validation:
python/e2e/test_context_managers.pyto verify correct context manager behavior, including automatic cleanup, error propagation, and nested context scenarios.python/test_client.pyto confirm that context manager methods return the correct values and propagate exceptions as expected.Internal improvements:
loggingandTracebackTypein relevant files to support error logging and context manager implementation. [1] [2]