Skip to content

Adding Transcription to Convert Functionality for Offline and Online #290

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

Merged
merged 31 commits into from
Jan 23, 2020

Conversation

kneekey23
Copy link
Contributor

Description of changes:
Transcription added under convert as an api call to both online and offline services supported by CoreML and AWS. In addition to a refactor in order for convert to take advantage of multiservice functionality that will change customer code upon receiving ConvertResult.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@kneekey23 kneekey23 added PullRequest predictions Issues related to the Predictions category labels Jan 14, 2020
@kneekey23 kneekey23 requested a review from royjit January 14, 2020 20:22
@kneekey23 kneekey23 self-assigned this Jan 14, 2020

class NativeWebSocketProvider: NSObject, AWSTranscribeStreamingWebSocketProvider, URLSessionWebSocketDelegate {
//swiftlint:disable weak_delegate
var clientDelegate: AWSTranscribeStreamingClientDelegate
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldnt this be weak?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can't be weak per the protocol from AWSTranscribeStreamingWebSocketProvider and I am not holding a reference to this delegate property from this class on any other class. I am instantiating a new class below that inherits from this protocol so I don't think it has to be weak in this case?

@kneekey23 kneekey23 marked this pull request as ready for review January 17, 2020 03:38
Copy link
Contributor

@royjit royjit left a comment

Choose a reason for hiding this comment

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

  • Still have concerns regarding concurrent requests.
    We create a websocket connection per request, but we donot keep track of it. NativeWebSocketProvider overrides each websocket tasks at line 31. I guess we need to keep track of each websocket connection so that, we can send the right endFrame messages to the right socket.

  • I have merged in another PR for the copyright files. You can rebase from master so that we can get rid of all the copyright year files.

@royjit
Copy link
Contributor

royjit commented Jan 22, 2020

  • Still have concerns regarding concurrent requests.
    We create a websocket connection per request, but we donot keep track of it. NativeWebSocketProvider overrides each websocket tasks at line 31. I guess we need to keep track of each websocket connection so that, we can send the right endFrame messages to the right socket.
  • I have merged in another PR for the copyright files. You can rebase from master so that we can get rid of all the copyright year files.

Decided to have convert() api for transcribe to have one request at a time and it doesnot support concurrent convert operation. So internally the streaming works as request/response model for now.

@kneekey23 kneekey23 merged commit 7847136 into master Jan 23, 2020
@lawmicha lawmicha deleted the kneekey/predictions-transcribe-3 branch May 11, 2020 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
predictions Issues related to the Predictions category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants