- Notifications
You must be signed in to change notification settings - Fork 109
fix: Logging - Downgrade 'tasks cancelled error' to debug-level logline #550
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Logging - Downgrade 'tasks cancelled error' to debug-level logline #550
Conversation
WalkthroughThe changes adjust the logging level for Changes
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (3.3.7)deepgram/clients/common/v1/abstract_async_websocket.pydeepgram/clients/agent/v1/websocket/async_client.pydeepgram/clients/listen/v1/websocket/async_client.py📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File ( |
| @jkroll-deepgram Thanks for putting this PR up. Here are some clarifications from Co-Pilot on this change and possible implictations. I want to ensure this the expected behavior we want and also validate some testing has been done on this change: Implications of the ChangeThe selected line changes the logging level for a specific exception: - self._logger.error("tasks cancelled error: %s", e) + self._logger.debug("tasks cancelled error: %s", e)Implications:
Testing RecommendationsTo ensure this change works as intended, the following types of tests should be considered: 1. Unit Testing
2. Integration Testing
Summary: |
| @jkroll-deepgram your solution doesn't solve the problem and, even if it would have, might have caused unintended issues elsewhere. I believe the correct fix would be to update abstract_async_websocket.py line 491 490: except asyncio.CancelledError as e: 491: self._logger.error("tasks cancelled error: %s", e) 492: self._logger.debug("AbstractAsyncWebSocketClient.finish LEAVE") 493: return TrueAdjusting the log from error to debug at this point fixes the problem and has no indirect effects. |
| @jkroll-deepgram 😅 I must apologize. I didn't pay attention to a modification I made during testing that made it look like your solution failed. After proper retesting, your solution passes. In addition, the additional fixes you made were made in the appropriate sections of code and shouldn't cause any unintended consequences. ✅ Passes Testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on @cpierce-deepgram validation this change should be ok to merge. cc @lukeocodes for awareness.
| @jkroll-deepgram LGTM @jpvajda this can be merged with a "fix: " prefix as-per the PR title |
bc2d160 to 77ab1a0 Compare | @jkroll-deepgram @cpierce-deepgram Thanks for the work on this! I'll merge this PR in and release it with the next SDK release. |
| Here is the script I used to test import asyncio import httpx import logging import os from dotenv import load_dotenv from deepgram.utils import verboselogs from deepgram import ( AsyncListenWebSocketClient, DeepgramClientOptions, LiveTranscriptionEvents, LiveOptions, ) # Load environment variables from .env file load_dotenv() # Fetch the Deepgram API key api_key = os.getenv("API_KEY") if not api_key: raise EnvironmentError("Missing API_KEY in environment variables or .env file.") # Setup verbose logging for debugging verboselogs.install() logger = logging.getLogger() logger.setLevel(logging.INFO) # Format log messages formatter = logging.Formatter( "[%(asctime)s] [%(levelname)s] [%(name)s] %(message)s", "%Y-%m-%d %H:%M:%S" ) console_handler = logging.StreamHandler() console_handler.setFormatter(formatter) logger.addHandler(console_handler) # URL of the audio stream source URL = "http://stream.live.vc.bbcmedia.co.uk/bbc_world_service" async def main(): """ Connects to a live audio stream, sends the stream to Deepgram in real-time, and prints out transcribed speech as it comes in. This function demonstrates how to use Deepgram's AsyncListenWebSocketClient with a live audio stream using httpx and asyncio. """ try: # Initialize Deepgram client with API key config = DeepgramClientOptions(api_key=api_key) # type: ignore dg_connection = AsyncListenWebSocketClient(config) # Handle transcription events async def on_message(self, result, **kwargs): sentence = result.channel.alternatives[0].transcript if sentence: print(f"speaker: {sentence}") # Register the event listener for transcripts dg_connection.on(LiveTranscriptionEvents.Transcript, on_message) # type: ignore # Set transcription options options = LiveOptions(model="nova-3") print("\n\nPress Enter to stop recording...\n\n") # Start the Deepgram connection success = await dg_connection.start(options) if not success: logger.error("Failed to start Deepgram connection.") return stop_event = asyncio.Event() async def stream_audio(): """ Streams audio from the given URL and sends it to Deepgram for transcription. """ try: logger.info("Creating HTTP client for stream...") async with httpx.AsyncClient() as client: async with client.stream("GET", URL, timeout=None) as response: logger.info(f"Connected to audio stream: {response.status_code}") async for chunk in response.aiter_bytes(): if stop_event.is_set(): logger.info("Stop signal received. Ending audio stream.") break await dg_connection.send(chunk) except asyncio.CancelledError: logger.warning("Audio streaming was cancelled.") except Exception as e: logger.exception(f"Error during audio streaming: {e}") # Start streaming audio in a background task audio_task = asyncio.create_task(stream_audio()) # Wait for user to press Enter await asyncio.get_event_loop().run_in_executor(None, input) # Signal the audio stream to stop and wait for task to finish stop_event.set() await audio_task # Cleanly close the Deepgram connection await dg_connection.finish() print("Transcription finished.") except Exception as e: logger.exception(f"Error during transcription: {e}") return if __name__ == "__main__": asyncio.run(main()) |
Proposed changes
Downgrade
tasks cancelled errorlog line to debug-level.Mitigates an issue reported by several customers: #501
That log line isn't really needed at error-level because it will always be raised when cancelling tasks.
Types of changes
What types of changes does your code introduce to the community Python SDK?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
Summary by CodeRabbit