streaming code used to hang when it was asked to "exit" - fixed now by puneith · Pull Request #635 · GoogleCloudPlatform/python-docs-samples · GitHub

@jerjou @jonparrott PTAL

Were you able to reproduce this bug? (I assume this is to address #608) I wasn't able to reproduce, which is why I hadn't responded to the bug yet..

Was this move necessary? I'd prefer to have the function that opened the stream be responsible for closing it.

I agree with you. But, the thread hangs and doesn't come out of the while loop. So, I had to move here and close when I get out.

Ah - this is because buff.put(None) is never executed if the above loop is exited because stoprequest.is_set() is True. (this means the blocking buff.get() in _audio_data_generator blocks forever, since it's never given the None to exit from its loop).

If you change the above loop to:

try:
    ...
except IOError:
    pass
finally:
    buff.put(None)

then you should be able to put this back.

No, this happens because the stream is stopped and the thread hangs when this thread is still in the loop.

Any particular reason to remove this line? It seems like good hygiene..

That might have been a mistake. I will restore it.

done

prefer .is_set, for python style conventions. Ditto below.

done

_audio_data_generator shouldn't need this. Once _fill_buffer gets the signal, it'll add None to the buffer, which will cause this generator to exit.

But, fill_buffer doesn't add the None unless it receives IOError which it doesn't. Ok let me make one change to _fill_buffer.

Actually to think of it making both threads stop from the same signal is better.

Can you explain your reasoning?

The reasoning: Both reader and writer threads consider the stopsignal. If reader looks at the indirect signal from the writer, we don't know if it needed to stop because there is really a stop signal or an error in the writer's output.

Hm... sure, okay, I can see that. I think there's a case for just letting _fill_buffer propagate the way it is, but I won't block on it.

I actually had thought about both cases, but can now also see the case for other option. And now as I am thinking more I think the other option makes more sense :) Let me go back to it.

Yes I was able to reproduce this. It did hang on my mac.

Please remove trailing whitespace

Hm... sure, okay, I can see that. I think there's a case for just letting _fill_buffer propagate the way it is, but I won't block on it.

Ah - this is because buff.put(None) is never executed if the above loop is exited because stoprequest.is_set() is True. (this means the blocking buff.get() in _audio_data_generator blocks forever, since it's never given the None to exit from its loop).

If you change the above loop to:

try:
    ...
except IOError:
    pass
finally:
    buff.put(None)

then you should be able to put this back.

Trailing whitespace

done

Any particular reason not to stop the stream before closing? While I'm not certain it's necessary, all the examples stop the stream before closing it.

Once the stream is stopped, one may not call write or read. And thread _fill_buffer gets stuck.

Without this, you'll have a race condition. ie if _audio_data_generator's buff.get() is waiting for data in the buffer when stoprequest is triggered, it could potentially wait forever and never yield a value.

Nice - thanks for catching that race condition 😁
Looks good - just some polishing of the comments left.

A comment here describing what putting None means would be helpful.

done

Can you make this comment describe what stoprequest is for? Simply repeating the name of the variable isn't helpful.

done

This comment should be right above the with block, since that's what it's describing.

done

Comments should describe the meaning and purpose of the code, not just narrate what the code is doing. Maybe something like:

# If `_fill_buffer` adds `None` to the buffer, the audio stream is closed.
# Yield the final bit of the buffer and exit the loop.

done

Ah - nice catch.

Sorry - I meant that this comment should be on the below with statement, where it was before. ie since the comment is describing the thread that collects audio data, it should be just before the record_audio call.

done

trancsription -> transcription

done

Line 219 and 220 are the same sentence (otherwise, line 220 is a sentence fragment). Please change to:

# stoprequest is an event object which is set in `listen_print_loop` to indicate that the transcription should be stopped.

done

I had a bunch of trouble parsing this sentence. Perhaps:

# The `_fill_buffer` thread checks this object, and closes the `audio_stream` once it's set.

done

Thanks!

@dpebot merge when green.

Okay! I'll merge when all statuses are green.

Does dpebot know he can also merge when someone approves the PR + green status?

I'm not sure what you're asking. Are you suggesting that we should merge PRs before the automated tests pass?

I am asking can dpebot take over when a) someone has approved the PR b) green status. Both a and b are true.

I don't understand what you're saying. Are you stating that - currently - someone has approved this PR AND the PR's status is green? Or are you asking whether dpebot can submit the PR if a OR b are true? Or are you asking whether dpebot can submit the PR when a AND b are true?

If it helps - IIUC, dpebot just waits for travis to turn green before submitting. Currently travis is still waiting for test results, so dpebot is not merging the PR yet.

I thought my question was simple :)

In other words I am asking. Do we have to ask dpebot explicitly "merge when everything is green"? Can dpebot not figure that out automatically and do the needful?

I think your question is simple, but you're not expressing the full question, or you're not giving enough context for the question, so it's hard to figure out what you're asking. Perhaps it'd be helpful to structure your questions into three steps:

What the current situation is
What you expect the situation to be
The question

For example:

We ask dpebot to merge when green
dpebot should be able to do this without us asking
Do we need to ask dpebot, when we want to merge?

It was ambiguous in your original question what you thought the current situation was, and what you expected the situation to be. For instance - what did you mean by 'also'? That implies that there's another situation wherein dpebot will merge, besides when it's approved + green; but you don't describe this other situation you allude to, so there's no way for me to know what you're contrasting.

Does that make sense?

To answer your question - dpebot listens for travis to complete, and only merges if it has status:success AND the automerge label on it. Telling dpebot to 'merge when green' simply adds the automerge label. We don't want dpebot to just merge all green PRs because eg the user might still be working on the PR (for example, addressing comments from the reviewer), even if the tests pass.

Wow! Travis is sloooow!

Yeah - it's usually one slow-running project in our org that backs everything else up :-/ We might consider changing to circleci, like some of the other teams are doing..

added streaming fix when exit

7b4bfc7

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 4, 2016

theacodes assigned jerjou Nov 4, 2016

puneith changed the title added streaming fix when exit streaming code used to hang when "exit" - fixed now Nov 4, 2016

puneith changed the title streaming code used to hang when "exit" - fixed now streaming code used to hang when it was asked to "exit" - fixed now Nov 4, 2016

jerjou suggested changes Nov 4, 2016

View reviewed changes

added audio_interface.terminate()

6e3a073

jerjou suggested changes Nov 4, 2016

View reviewed changes

removed audio_stream stop

1559d95

jerjou suggested changes Nov 5, 2016

View reviewed changes

puneithk added 2 commits November 4, 2016 19:35

removed trailing whitespace

d3be2f8

added None and checked for None in _audio_data_generator

a5d26c9

jerjou suggested changes Nov 7, 2016

View reviewed changes

added elaborate comments in the code

c0f8e5a

jerjou suggested changes Nov 7, 2016

View reviewed changes

fixed comments in the code

9648066

jerjou approved these changes Nov 7, 2016

View reviewed changes

puneith closed this Nov 7, 2016

puneith reopened this Nov 7, 2016

dpebot added the automerge Merge the pull request once unit tests and other checks pass. label Nov 7, 2016

puneithk added 2 commits November 7, 2016 14:40

fixed comments

a891544

added 'the' back

decf575

dpebot merged commit 08b4da6 into master Nov 8, 2016

dpebot deleted the streaming_fix branch November 8, 2016 02:56

jerjou mentioned this pull request Nov 9, 2016

Use pyaudio's built-in async api. #645

Merged

-                  audio_stream.stop_stream()
-                  audio_stream.close()
                   fill_buffer_thread.join()
-                  audio_interface.terminate()

                   """
-                  while True:
-                      # Use a blocking get() to ensure there's at least one chunk of data
+                  while not stoprequest.isSet():

-                  while True:
-                      # Use a blocking get() to ensure there's at least one chunk of data
+                  while not stoprequest.is_set():
+                      # Use a blocking get() to ensure there's at least one chunk of data

                   """
                   while True:
-                      # Use a blocking get() to ensure there's at least one chunk of data
+                      # Use a blocking get() to ensure there's at least one chunk of data

@@ @@ -208,7 +212,11 @@ def main(): @@
                           make_channel('speech.googleapis.com', 443)) as service:
                       # For streaming audio from the microphone, there are three threads.
                       # First, a thread that collects audio data as it comes in



		def _audio_data_generator(buff):
		def _audio_data_generator(buff, stoprequest):

                       # First, a thread that collects audio data as it comes in
-                      with record_audio(RATE, CHUNK) as buffered_audio_data:
+                      # stop request

                           except queue.Empty:
                               break
+                      # If the data contains None then set stop = True.

-                      with record_audio(RATE, CHUNK) as buffered_audio_data:
+                      # stoprequest is event object which is set in `listen_print_loop`.
+                      # To indicate that the trancsription should be stopped.

streaming code used to hang when it was asked to "exit" - fixed now by puneith · Pull Request #635 · GoogleCloudPlatform/python-docs-samples · GitHub

Uh oh!

streaming code used to hang when it was asked to "exit" - fixed now - #635

streaming code used to hang when it was asked to "exit" - fixed now#635dpebot merged 9 commits intomasterGoogleCloudPlatform/python-docs-samples:masterfrom streaming_fixGoogleCloudPlatform/python-docs-samples:streaming_fixCopy head branch name to clipboard

Conversation

puneith commented Nov 4, 2016

Uh oh!

jerjou left a comment

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!

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!

puneith commented Nov 4, 2016

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!

jerjou left a comment

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

streaming code used to hang when it was asked to "exit" - fixed now#635
dpebot merged 9 commits into
masterfrom
streaming_fix

puneith commented Nov 7, 2016 •
edited
Loading