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

Audio streaming training with masking #148

Merged
merged 15 commits into from
Nov 20, 2024
Merged

Conversation

saeeddhqan
Copy link
Contributor

By utilizing a mask, we can simulate audio streaming.

Copy link
Contributor

@farzadab farzadab left a comment

Choose a reason for hiding this comment

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

Thanks Saeed. I left some comments.

It would be ideal to add some tests, for example to make sure the structure of the mask matches what we expect. If you can think of another test feel free to add one.
I know the repo doesn't have many tests, but we'd like to change that.

ultravox/training/config_base.py Outdated Show resolved Hide resolved
ultravox/model/ultravox_model.py Outdated Show resolved Hide resolved
ultravox/model/ultravox_model.py Outdated Show resolved Hide resolved
ultravox/model/ultravox_model.py Outdated Show resolved Hide resolved
ultravox/model/ultravox_model.py Outdated Show resolved Hide resolved
ultravox/model/ultravox_model.py Outdated Show resolved Hide resolved
@saeeddhqan
Copy link
Contributor Author

@farzadab Hi,
I created a test file for ultravox_model, moved the function, and changed var names.
I look forward to further improvements.

Copy link
Contributor

@farzadab farzadab left a comment

Choose a reason for hiding this comment

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

Thanks. I left a few nit comments.
Please make sure to run just format to run formatting and use just test to make sure the tests pass.

ultravox/model/ultravox_model.py Outdated Show resolved Hide resolved
ultravox/model/ultravox_model.py Outdated Show resolved Hide resolved
ultravox/model/ultravox_model.py Outdated Show resolved Hide resolved
ultravox/training/configs/streaming_tinyllama.yaml Outdated Show resolved Hide resolved
ultravox/model/ultravox_model_test.py Outdated Show resolved Hide resolved
ultravox/model/ultravox_model.py Outdated Show resolved Hide resolved
@saeeddhqan
Copy link
Contributor Author

@farzadab, I utilized 'just test', and 'just format', and corrected mentioned issues.

Copy link
Contributor

@farzadab farzadab left a comment

Choose a reason for hiding this comment

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

Assuming the tests pass it LGTM. Thanks Saeed!

Copy link
Contributor

@farzadab farzadab left a comment

Choose a reason for hiding this comment

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

Seems like just test failed since mypy is complaining.

I proposed the solution for two of the issues. The last one I couldn't figure out at a glance why the value could be None, but it should be easy to figure out with the code editor's help.

ultravox/training/config_base.py Outdated Show resolved Hide resolved
ultravox/model/ultravox_model.py Outdated Show resolved Hide resolved
@saeeddhqan
Copy link
Contributor Author

@farzadab solved.

@farzadab farzadab merged commit 9fc2732 into fixie-ai:main Nov 20, 2024
1 check passed
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