bidi - remove python 3.11+ features #1302
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| raise ExceptionGroup("failed stop sequence", exceptions) | ||
| exceptions.append(RuntimeError("failed stop sequence")) | ||
| for i in range(1, len(exceptions)): | ||
| exceptions[i].__cause__ = exceptions[i - 1] |
There was a problem hiding this comment.
The exception traceback will show all the causes chained together. We have this unit tested down below.
There was a problem hiding this comment.
This is slightly misleading right? e.g. in reality they didn't really all cause each other, we're just presenting it that way?
Should we indicate that somehow? (Like via the message or something)
Can we also document this in the code
There was a problem hiding this comment.
Ah shoot, I actually meant to get a traceback message that looks something like:
Traceback (most recent call last):
File "/Users/pgrayy/Projects/Strands/sdk-python/src/strands/experimental/bidi/_async/__init__.py", line 24, in stop_all
await func()
File "/Users/pgrayy/Library/Application Support/hatch/env/virtual/.pythons/3.12/python/lib/python3.12/unittest/mock.py", line 2291, in _execute_mock_call
raise effect
ValueError: stop 2 failed
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/Users/pgrayy/Projects/Strands/sdk-python/src/strands/experimental/bidi/_async/__init__.py", line 24, in stop_all
await func()
File "/Users/pgrayy/Library/Application Support/hatch/env/virtual/.pythons/3.12/python/lib/python3.12/unittest/mock.py", line 2291, in _execute_mock_call
raise effect
ValueError: stop 4 failed
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/Users/pgrayy/Projects/Strands/sdk-python/tests/strands/experimental/bidi/_async/test__init__.py", line 17, in test_stop_exception
await stop_all(func1, func2, func3, func4)
File "/Users/pgrayy/Projects/Strands/sdk-python/src/strands/experimental/bidi/_async/__init__.py", line 33, in stop_all
raise exceptions[-1]
RuntimeError: failed stop sequenceNote, this says During handling of the above exception, another exception occurred instead of The above exception was the direct cause of the following exception. I would just need to switch __cause__ to __context__.
What would you think of this?
| @@ -1,3 +1,4 @@ | |||
| import traceback | |||
There was a problem hiding this comment.
There are some lint fixes in these 2 test files. They weren't caught previously because there is a bug in our lint command used specifically for bidi. I am addressing this as part of #1299 which resolves the issue by checking bidi using the existing hatch scripts we have configured for the rest of strands.
…nto bidi-remove-311-features
| try: | ||
| await asyncio.gather(inputs_task, outputs_task) | ||
| except (Exception, asyncio.CancelledError) as error: | ||
| inputs_task.cancel() | ||
| outputs_task.cancel() | ||
| await asyncio.gather(inputs_task, outputs_task, return_exceptions=True) | ||
| if not isinstance(error, asyncio.CancelledError): | ||
| raise |
There was a problem hiding this comment.
Few things to note:
- It is okay to call cancel on an already cancelled asyncio task.
- We are ignoring the exceptions from this second gather. This second gather is just waiting on the tasks to finish after explicit cancellation. We want to make sure to instead raise what lead us to here from the first gather call if an Exception.
There was a problem hiding this comment.
The second bulletpoint is a good call-out for the code.
An explaining comment would be good overall for this chunk - including (maybe) the original code if we were on 3.12
There was a problem hiding this comment.
Will document. And maybe what else I'll do is wrap this logic up into our own implementation of a "TaskGroup" and place it into our _async subpackage.
| raise ExceptionGroup("failed stop sequence", exceptions) | ||
| exceptions.append(RuntimeError("failed stop sequence")) | ||
| for i in range(1, len(exceptions)): | ||
| exceptions[i].__cause__ = exceptions[i - 1] |
There was a problem hiding this comment.
This is slightly misleading right? e.g. in reality they didn't really all cause each other, we're just presenting it that way?
Should we indicate that somehow? (Like via the message or something)
Can we also document this in the code
| try: | ||
| await asyncio.gather(inputs_task, outputs_task) | ||
| except (Exception, asyncio.CancelledError) as error: | ||
| inputs_task.cancel() | ||
| outputs_task.cancel() | ||
| await asyncio.gather(inputs_task, outputs_task, return_exceptions=True) | ||
| if not isinstance(error, asyncio.CancelledError): | ||
| raise |
There was a problem hiding this comment.
The second bulletpoint is a good call-out for the code.
An explaining comment would be good overall for this chunk - including (maybe) the original code if we were on 3.12
| await model4.start() | ||
| mock_ws.close.side_effect = Exception("Close failed") | ||
| with pytest.raises(ExceptionGroup): | ||
| with pytest.raises(RuntimeError, match=r"failed stop sequence"): |
There was a problem hiding this comment.
Do we have guidelines on when we use a custom exception vs general?
There was a problem hiding this comment.
We don't have guidelines and probably should. In this particular case, users wouldn't have reason to act specifically on this error which is why I made it a RuntimeError. They would just likely catch this generally with except Exception because it is an unrecoverable internal error. That is in comparison to something like BidiModelTimeoutError, which we created so that we could trigger the restart connection workflow internally.
So if we are to develop guidelines, I would probably start with something like this. Would the exception be used to drive a specific action/workflow. If not and the user would just end up catching it with except Exception, then maybe we don't require a custom error. I would have to think about this some more, but this is where my head was at for this particular case.
|
|
||
| run_task = asyncio.current_task() | ||
| if run_task and run_task.cancelling() > 0: # externally cancelled | ||
| raise |
There was a problem hiding this comment.
As an example here, if the user is running run through asyncio.create_task and calls task.cancel() themselves, we want that CancelledError propagating. Similarly for KeyboardInterrupt. That is what TaskGroup does.
Internal cancellations however, like with the inputs_task.cancel() inside run_outputs, we don't want that to reraise to the user.
2 participants
Description
Removing the python 3.11+ features used directly in the bidi subpackage. Specifically I am replacing use of:
asyncio.TaskGroup: Used inBidiAgent.run. Replacing with manual task cancellation management.ExceptionGroup: Used in_async.stop_all. Replacing with chainedRuntimeError.ExceptionGroup. More likely, if they are catching unexpected, internal errors, they would do so withException. So for all intents and purposes, I would call this a backwards compatible change. Regardless, bidi is experimental.Note, this is part of a larger effort to have bidi run in Python 3.10 and 3.11. Only the
BidiNovaSonicModelprovider requires Python 3.12+. The rest of bidi however should be runnable in all other versions of Python that Strands as a whole supports.Related Issues
#1299
Documentation PR
N/A
Type of Change
Other (please describe): On the way to supporting 3.10 and 3.11 for bidi.
Testing
How have you tested the change? Verify that the changes do not break functionality or introduce warnings in consuming repositories: agents-docs, agents-tools, agents-cli
hatch run preparehatch run bidi:prepare: Updated some existing unit tests.Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.