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

Fix the sleep logic for streaming as per sample rate #279

Merged
merged 11 commits into from
Aug 4, 2016
Merged

Fix the sleep logic for streaming as per sample rate #279

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
80eb6da
sleep 100ms instead of sleeping as function of rate
puneithk Jul 28, 2016
7d8f4b5
added sleep as inverse of sampling rate
puneithk Jul 29, 2016
d87d691
added LINEAR16 bytes per sample constant
puneithk Jul 29, 2016
ff7ffbc
added few variables
puneithk Jul 29, 2016
a23abdc
added truth and log4j
puneithk Aug 4, 2016
0cee557
added 32khz audio
puneithk Aug 4, 2016
df11062
added Streaming Test
puneithk Aug 4, 2016
a253ebf
added unit tests for 16 and 32 khz audio
puneithk Aug 4, 2016
d1fdc61
checkstyle fixes
puneithk Aug 4, 2016
a44e692
renamed to KHz
puneithk Aug 4, 2016
4ac456e
added comment
puneithk Aug 4, 2016
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
16 changes: 12 additions & 4 deletions speech/grpc/src/main/java/com/examples/cloud/speech/StreamingRecognizeClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,13 @@
import java.util.List;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
import java.util.logging.Level;
import java.util.logging.Logger;
import org.apache.log4j.ConsoleAppender;
import static org.apache.log4j.ConsoleAppender.SYSTEM_OUT;
import org.apache.log4j.Level;
import org.apache.log4j.Logger;
import org.apache.log4j.SimpleLayout;
import java.io.OutputStreamWriter;


/**
* Client that sends streaming audio to Speech.Recognize and returns streaming transcript.
Expand Down Expand Up @@ -76,6 +81,10 @@ public StreamingRecognizeClient(ManagedChannel channel, String file, int samplin
this.channel = channel;

speechClient = SpeechGrpc.newStub(channel);

//Send log4j logs to Console
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be configured in a configuration file, rather than in code? eg if a user wanted to log to a file instead of stdout..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It generally is, but I thought for the purpose of sample was bit of an overkill. But I can change if folks have other thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think letting the logger log to the default destination is fine for the sample. If there's a reason that messages should go to stdout, then it should print to stdout rather than logging.

Semantics are important for samples. If you're logging something, then it should be something that shows up in logs. If you're printing something, then it should be communicating with the user. Sending logs to stdout sends sort of mixed messages, IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the problem here is different. Which is we are inspecting the logs to evaluate the tests, which is not a generally we should do. Its because of this reason. Now, we shouldn't do System.out.println in the Streaming sample and therefore we have to do this trick in the JUnit Tests. One way I think of fixing this is by having the recognize method return a Stream so tha JUnit inspects the Stream and now you see its getting bit involved :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh interesting...
So this script doesn't even display any output, it just logs the response... -_-; This strikes me as a bit weird, but I realize it's tangential to this CL, so I won't block on it. I do like your idea about returning a Stream, but yeah. Another time :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Usually, we just configure this with logging.properties, you could comment. If you are going to run this on GCE, you might wish to integrate w/ gcloud-java's logging.

ConsoleAppender appender = new ConsoleAppender(new SimpleLayout(), SYSTEM_OUT);
logger.addAppender(appender);
}

public void shutdown() throws InterruptedException {
Expand All @@ -94,8 +103,7 @@ public void onNext(StreamingRecognizeResponse response) {

@Override
public void onError(Throwable error) {
Status status = Status.fromThrowable(error);
logger.log(Level.WARNING, "recognize failed: {0}", status);
logger.log(Level.WARN, "recognize failed: {0}", error);
finishLatch.countDown();
}

Expand Down
42 changes: 7 additions & 35 deletions speech/grpc/src/test/java/com/examples/cloud/speech/StreamingRecognizeClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
import org.apache.log4j.WriterAppender;
import org.apache.log4j.Logger;
import org.apache.log4j.Level;
import org.apache.log4j.PatternLayout;
import org.apache.log4j.SimpleLayout;
import org.apache.log4j.spi.LoggingEvent;
import org.apache.log4j.AppenderSkeleton;
import java.io.Writer;
Expand All @@ -52,11 +52,13 @@
*/
@RunWith(JUnit4.class)
public class StreamingRecognizeClientTest {
private TestAppender appender;
private Writer writer;
private WriterAppender appender;

@Before
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a @BeforeClass, since it's adding an appender (rather than replacing it)? ie if this ones once per test, won't new appenders get added as many times as there are tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have to do it for each test so that StringWriter is new. There is an After which I now think should be AfterClass where all added appenders will be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see. No - having it in an After makes sense. Sorry - didn't see the After.

public void setUp() {
appender = new TestAppender();
writer = new StringWriter();
appender = new WriterAppender(new SimpleLayout(), writer);
Logger.getRootLogger().addAppender(appender);
}

Expand All @@ -76,7 +78,7 @@ public void test16KHzAudio() throws InterruptedException, IOException {
StreamingRecognizeClient client = new StreamingRecognizeClient(channel, path.toString(), 16000);

client.recognize();
assertThat(appender.getLog()).contains("transcript: \"how old is the Brooklyn Bridge\"");
assertThat(writer.toString()).contains("transcript: \"how old is the Brooklyn Bridge\"");
}

@Test
Expand All @@ -90,36 +92,6 @@ public void test32KHzAudio() throws InterruptedException, IOException {
StreamingRecognizeClient client = new StreamingRecognizeClient(channel, path.toString(), 32000);

client.recognize();
assertThat(appender.getLog()).contains("transcript: \"how old is the Brooklyn Bridge\"");
}

/**
*
* TestAppender for JUnit tests to check logger output
*/
class TestAppender extends AppenderSkeleton {
private final List<LoggingEvent> loggingEvents = new ArrayList<LoggingEvent>();

@Override
public boolean requiresLayout() {
return false;
}

@Override
public void close() {}

@Override
protected void append(final LoggingEvent loggingEvent) {
loggingEvents.add(loggingEvent);
}

public String getLog() {
StringBuilder builder = new StringBuilder();
for(LoggingEvent event : loggingEvents) {
builder.append(event.getMessage().toString());
builder.append("\n");
}
return builder.toString();
}
assertThat(writer.toString()).contains("transcript: \"how old is the Brooklyn Bridge\"");
}
}