Skip to content
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 memory creation task on stream_transcript #997

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

beastoin
Copy link
Collaborator

@beastoin beastoin commented Oct 8, 2024

Summary by Entelligence.AI

  • Refactor: Enhanced the stream_transcript function in transcribe_v2.py for better task management and error handling. This update improves the stability and reliability of the transcription service.
  • Refactor: Updated the receive_audio function to optimize socket usage, enhancing the performance and efficiency of audio data transmission.

Copy link

github-actions bot commented Oct 8, 2024

Image description Entelligence.AI

Walkthrough

This update refactors the stream_transcript function in the backend router, improving the handling of memory creation tasks using asyncio. It introduces a new function, create_memory_creation_task, to streamline task management. The changes also enhance the use of sockets in the receive_audio function.

Changes

File Summary
backend/routers/transcribe_v2.py Refactored the stream_transcript function to improve memory creation task handling with asyncio. Introduced a new function create_memory_creation_task for better task management. Enhanced socket usage in the receive_audio function.

🎉 With every refactor, we grow,

In the codebase, improvements show.

Memory tasks now smoothly flow,

And sockets in harmony glow. 🌟


Uplevel your code reviews with Entelligence.AI Pro

Entelligence.AI Pro

If you like this project, please support us by purchasing the Pro version. The Pro version has advanced context, superior noise reduction and several proprietary improvements compared to the open source version. Moreover, Entelligence.AI Pro is free for open source projects.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

Commits Files that changed from the base of the PR and between b491083 and 644ac38 commits.
Files selected (1)
  • backend/routers/transcribe_v2.py (3)
Review comments generated (4)
  • Review: 3
  • LGTM: 1

Tips

Chat with Image description Entelligence.AI Bot (@Entelligence.AI)

  • Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
  • Invite the bot into a review comment chain by tagging @Entelligence.AI in a reply.

Code suggestions

  • The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
  • You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.

Pausing incremental reviews

  • Add @Entelligence.AI: ignore anywhere in the PR description to pause further reviews from the bot.

Comment on lines +210 to +221
async def create_memory_creation_task():
nonlocal memory_creation_task

if memory_creation_task is not None:
await memory_creation_task.cancel()
try:
await memory_creation_task
except asyncio.CancelledError:
print("memory_creation_task is cancelled now")

memory_creation_task = asyncio.create_task(_trigger_create_memory_with_delay(memory_creation_timeout))

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The create_memory_creation_task function is a good addition for managing the memory creation task. However, there's an issue with the cancellation of the task. The cancel() method only requests cancellation and doesn't guarantee it. It would be better to handle this in a more robust way by checking if the task was cancelled successfully.

        if memory_creation_task is not None:
            memory_creation_task.cancel()
            try:
                await memory_creation_task
            except asyncio.CancelledError:
                print("memory_creation_task is cancelled now")
+            if not memory_creation_task.cancelled():
+                print("Failed to cancel memory_creation_task")

if memory_creation_task is not None:
memory_creation_task.cancel()
memory_creation_task = asyncio.create_task(_trigger_create_memory_with_delay(memory_creation_timeout))
asyncio.run_coroutine_threadsafe(create_memory_creation_task(), loop)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

This line uses asyncio.run_coroutine_threadsafe which is generally used when you want to schedule a coroutine from a different thread. If this code is running within an event loop, it would be more appropriate to use asyncio.create_task or loop.create_task. Please ensure that this usage is necessary and correct in your context.

-        asyncio.run_coroutine_threadsafe(create_memory_creation_task(), loop)
+        loop.create_task(create_memory_creation_task())

Comment on lines 342 to 348
if speechmatics_socket1 is not None:
await speechmatics_socket1.send(data)

if deepgram_socket is not None:
if dg_socket1 is not None:
elapsed_seconds = time.time() - timer_start
if elapsed_seconds > speech_profile_duration or not dg_socket2:
dg_socket1.send(data)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [345-349]

There seems to be a change in variable name from deepgram_socket to dg_socket1. This change itself is not an issue, but please make sure that all references to deepgram_socket are updated to dg_socket1 to maintain consistency and avoid potential NameErrors.

@beastoin beastoin merged commit 7eb2bd0 into main Oct 8, 2024
1 check passed
@beastoin beastoin deleted the krown_fixes_memory_creation_task branch October 8, 2024 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant