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.

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

Enhancements to ASR API #5

Merged
merged 58 commits into from
Jun 23, 2020
Merged

Enhancements to ASR API #5

merged 58 commits into from
Jun 23, 2020

Conversation

ar13pit
Copy link
Contributor

@ar13pit ar13pit commented Apr 28, 2020

This PR:

  1. Decouples all the elements of the ASR pipeline.
  2. Unifies the API of each element of the ASR pipeline.
  3. Introduces a new AsrPipeline class to construct a pipeline using appropriate elements. This would serve as a manager of the pipeline and its methods like open, start, stop, close would act as the controlling interfaces to the pipeline.

Each element of the ASR pipeline can still be used standalone by supplying the appropriate input using the method next_chunk of the element. Supplying input to Source elements using next_chunk would do nothing as they generate data, but to keep an unified API they accept input.

Completes a part of #4

Appropriate changes need to be made in tue-robotics-graveyard/yapykaldi_ros#1

LoyVanBeek
LoyVanBeek previously approved these changes Apr 30, 2020
@ar13pit
Copy link
Contributor Author

ar13pit commented Apr 30, 2020

@LoyVanBeek This wasn't ready :)

@LoyVanBeek
Copy link
Contributor

Oh hey, that's weird. Looked OK but didn't see it was a draft

@ar13pit ar13pit marked this pull request as ready for review May 20, 2020 19:27
@ar13pit ar13pit requested a review from LoyVanBeek May 20, 2020 19:27
@ar13pit
Copy link
Contributor Author

ar13pit commented May 20, 2020

@LoyVanBeek With this I can say, the API can almost be treated as stable. I have tested the changes with test_asr.py and test_io.py.

@ar13pit ar13pit mentioned this pull request May 20, 2020
4 tasks
"""Optional method to stop the stream of the element"""

def register_callback(self, callback):
"""Register a callback to the element outside the pipeline"""
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 either implemented or raise NotImplementedError?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we don't have this implemented then by default it is pass. But do you suggest to add a NotImplementedError ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is pass in a valid behavior, could there be a class that works by using pass? If not, then use NotImplementedError. Otherwise, passis a valid default implementation.

from ..logger import logger


class AsrPipeline(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this an AsrPipelineElementBase subclass as well for extra software architecture hard-on :-).

It matches the API already and allows for nesting, which is always cool somehow 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha yeah! I had to think a lot for making it like this. Will subclass it.

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'm going to make this into an issue as there are some attributes that need cleanup by converting it into a property

@LoyVanBeek
Copy link
Contributor

Bump

@ar13pit ar13pit requested a review from LoyVanBeek June 23, 2020 19:20
@ar13pit ar13pit merged commit 334a34a into master Jun 23, 2020
@ar13pit ar13pit deleted the feature/split_sink branch June 23, 2020 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants