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.

Sign up for GitHub

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

Jump to bottom

Fix memory creation task on stream_transcript #997

Merged
merged 1 commit into from
Oct 8, 2024
Merged

Fix memory creation task on stream_transcript #997

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions backend/routers/transcribe_v2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,10 +207,21 @@ def _get_or_create_in_progress_memory(segments: List[dict]):
redis_db.set_in_progress_memory_id(uid, memory.id)
return memory

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))

Comment on lines +210 to +221
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")

def stream_transcript(segments, _):
nonlocal websocket
nonlocal seconds_to_trim
nonlocal memory_creation_task

if not segments or len(segments) == 0:
return
Expand All @@ -219,9 +230,7 @@ def stream_transcript(segments, _):
if seconds_to_trim is None:
seconds_to_trim = segments[0]["start"]

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())


# Segments aligning duration seconds.
if seconds_to_add:
Expand Down Expand Up @@ -333,7 +342,7 @@ async def receive_audio(dg_socket1, dg_socket2, soniox_socket, speechmatics_sock
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)
Comment on lines 342 to 348
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.

Expand Down
Loading