fix-pushtotalk #206

PizlaTheDeveloper · 2018-03-18T11:14:31Z

Suggesting changes made on pushtotalk.py to be able to use fix-stream #188 by @proppy

googlebot · 2018-03-18T11:14:34Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

PizlaTheDeveloper · 2018-03-18T11:29:23Z

I signed it!

googlebot · 2018-03-18T11:29:25Z

CLAs look good, thanks!

proppy · 2018-03-19T02:51:19Z

@PizlaTheDeveloper Thanks for reporting the issue, and working on a fix!

I still get this error when running your branch:

ERROR:root:Exception iterating requests!
Traceback (most recent call last):
  File "/usr/local/google/home/proppy/src/devrel/samples/assistant/embedded-sdk-python/env/lib/python3.5/site-packages/grpc/_channel.py", line 187, in consume_request_iterator
    request = next(request_iterator)
  File "googlesamples/assistant/grpc/pushtotalk.py", line 124, in iter_assist_requests
    for c in self.gen_assist_requests():
  File "googlesamples/assistant/grpc/pushtotalk.py", line 202, in gen_assist_requests
    for data in self.conversation_stream:
  File "/usr/local/google/home/proppy/src/devrel/samples/assistant/embedded-sdk-python/google-assistant-sdk/googlesamples/assistant/grpc/audio_helpers.py", line 329, in <lambda>
    return iter(lambda: self.read(self._iter_size), b'')
  File "/usr/local/google/home/proppy/src/devrel/samples/assistant/embedded-sdk-python/google-assistant-sdk/googlesamples/assistant/grpc/audio_helpers.py", line 310, in read
    return self._source.read(size)
  File "/usr/local/google/home/proppy/src/devrel/samples/assistant/embedded-sdk-python/google-assistant-sdk/googlesamples/assistant/grpc/audio_helpers.py", line 198, in read
    buf, overflow = self._audio_stream.read(size)
  File "/usr/local/google/home/proppy/src/devrel/samples/assistant/embedded-sdk-python/env/lib/python3.5/site-packages/sounddevice.py", line 1107, in read
    _check(err)
  File "/usr/local/google/home/proppy/src/devrel/samples/assistant/embedded-sdk-python/env/lib/python3.5/site-packages/sounddevice.py", line 2561, in _check
    raise PortAudioError(errormsg, err, hosterror_info)
sounddevice.PortAudioError: <unprintable PortAudioError object>

starting on the second request.

proppy · 2018-03-19T02:52:27Z

google-assistant-sdk/googlesamples/assistant/grpc/pushtotalk.py

    if input_audio_file:
        audio_source = audio_helpers.WaveSource(
            open(input_audio_file, 'rb'),
            sample_rate=audio_sample_rate,
            sample_width=audio_sample_width
        )
    else:
-        audio_source = audio_device = (
-            audio_device or audio_helpers.SoundDeviceStream(
+        audio_source = audio_helpers.SoundDeviceStream(


if we don't reuse the same stream for input/output, I think we might be able to get rid of some of the locking happening in conversationstream.

Maybe reusing the stream is possible, I didn't understand how it could be reused, but now it is more clear for me :) Sorry, I am Python beginner :)

proppy · 2018-03-19T06:36:33Z

google-assistant-sdk/googlesamples/assistant/grpc/pushtotalk.py

@@ -138,6 +137,7 @@ def iter_assist_requests():
                logging.info('Transcript of user request: "%s".',
                             ' '.join(r.transcript
                                      for r in resp.speech_results))
+                self.conversation_stream.start_playback()


I think we want to keep doing this in the same grpc thread from where the stream reading is happening (i.e: at the end of the generator).

I think the whole problem is that I get data for playback in assist response before the request generator ends, so the playback is not started yet and writing to playback stream gets stuck while waiting to start the playback...

@PizlaTheDeveloper yep, but I don't think this will be an issue if we use two different stream as you introduced with d0eb6a3

Can you give a try to 2c63ebc and let me know if it works for you?

Sorry, but it doesn't work :(
The generator 'iter_assist_requests' will never stop iterating because it iterates while conversation_stream has data on its input, but there will be always some data if the source stream is running. So the recording will never stop.

I think that the events '_stop_recording' and '_start_playback ' should stay in ConversationStream to be able to signalize stop recording and return empty data to read calling what will end the iteration. Stop recording must be signaled when END_OF_UTTERANCE occurs.

I don't know if I described it correctly :)

I think my previous code was correct :)

Thank you for your interest

The generator 'iter_assist_requests' will never stop iterating because it iterates while conversation_stream has data on its input, buend_of_utterancet there will be always some data if the source stream is running. So the recording will never stop.

Makes total sense!

I added an explicit end_of_utterance event in 187b485 (which I believe is cleaner that tying everything to stop_recording) let me know what you think!

@PizlaTheDeveloper Thanks for digging this out.

Because of https://github.com/grpc/grpc/blob/16316a9fec3a78baf83d96dbdb392e90226bb013/src/python/grpcio/grpc/_channel.py#L682, I was making the assumption that grpc would always exhaust the request generator before ending the iteration of the response, and thought this would always be called:
187b485#diff-7262ae17d2cea4f6cb6c2b5934aaf939L127

But it looks like it's not the case! and that the iteration on self.assistant.Assist responses can finish before iter_assist_requests is exhausted. (Maybe @nathanielmanistaatgoogle can shed some light on the expected behavior).

@PizlaTheDeveloper can you give a try at 2dfc868 I added explicit locking to make sure we can safely close the recording stream from the main thread.

It's working like a charm now !!!

can you review/approve #188 ?

Without reading this whole conversation, so I may be missing some context: yes, it is not the case that exhaustion of the request iterator is guaranteed. If an RPC ends (for whatever reason, successfully or unsuccessfully) before exhaustion of the request iterator, iteration of the request iterator may be stopped. (Or it may be continued! Don't depend on either being the case.)

@nathanielmanistaatgoogle Thanks for the confirmation! So we really shouldn't perform any cleanup/teardown logic at the end of the iterator :)

fix-pushtotalk

d0eb6a3

proppy reviewed Mar 19, 2018

View reviewed changes

proppy merged commit d0eb6a3 into googlesamples:fix-stream Mar 19, 2018

proppy reviewed Mar 19, 2018

View reviewed changes

proppy mentioned this pull request Mar 19, 2018

google-assistant-sdk/pushtotalk: fix conversation_stream handling #188

Merged

fix-pushtotalk #206

fix-pushtotalk #206

Uh oh!

Conversation

PizlaTheDeveloper commented Mar 18, 2018

Uh oh!

googlebot commented Mar 18, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

PizlaTheDeveloper commented Mar 18, 2018

Uh oh!

googlebot commented Mar 18, 2018

Uh oh!

proppy commented Mar 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!