tts-ros1
tts-ros1 copied to clipboard
Caching files
Issue: #19
Description of changes: Added support for caching files by saving them and keeping note of them in sqlite db. Cache file size is tracked and when a maximum is exceeded, the least recently used file is deleted.
TODO
- [ ] Expose the cache size to the launch system
- [ ] Allow the cache location to be set in the launch system
- [ ] Test more widely (only tested with the simple action server example)
- [ ] Document all of this for general use
- [ ] Add unit tests for this functionality
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Codecov Report
Merging #36 (ee320dd) into master (c8ebbf7) will increase coverage by
4.96%
. The diff coverage is94.68%
.
:exclamation: Current head ee320dd differs from pull request most recent head b3829ee. Consider uploading reports for the commit b3829ee to get more accurate results
@@ Coverage Diff @@
## master #36 +/- ##
==========================================
+ Coverage 77.45% 82.41% +4.96%
==========================================
Files 3 4 +1
Lines 275 364 +89
==========================================
+ Hits 213 300 +87
- Misses 62 64 +2
Flag | Coverage Δ | |
---|---|---|
ROS_1 | 82.41% <94.68%> (+4.96%) |
:arrow_up: |
kinetic | 82.41% <94.68%> (+4.96%) |
:arrow_up: |
melodic | 82.41% <94.68%> (+4.96%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
Impacted Files | Coverage Δ | |
---|---|---|
tts/src/tts/db.py | 86.84% <86.84%> (ø) |
|
tts/src/tts/synthesizer.py | 92.53% <100.00%> (+8.19%) |
:arrow_up: |
tts/src/tts/__init__.py | 96.42% <0.00%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update dfd3802...b3829ee. Read the comment docs.
I can't get the code coverage up because of #37 . When those tests do start running, I expect that it will be a good idea to add random strings to the end of any test text that need to be pinging the server. Otherwise repeated runs of tests will just hit the cache. There also need to be some tests which intentionally hit the cache.
@AAlon is this something that you all are interested in? Should I keep working on it?
@AAlon is this something that you all are interested in? Should I keep working on it?
Definitely! thank you for contributing. We'll be reviewing this soon.
@cevans87 Thanks for the feedback, sorry for being slow to get back on it. I have gotten rid of the size table per your recommendation and pulled some stuff in the db class to improve readability. I have tested under both normal operation and deleting cached files from the disk without telling the db.
I just found a bug with this: If there is no network connection, then the error utterance (' the polly service cannot be reached ...') gets cached as whatever is being said. Then the system will always say that when the text is being said.
I just found a bug with this: If there is no network connection, then the error utterance (' the polly service cannot be reached ...') gets cached as whatever is being said. Then the system will always say that when the text is being said.
Are you actively working on that? if so we should probably wait before re-reviewing. It'd be great if you could add some unit tests too. Thanks!
@AAlon I took a look at it a while back, but haven't had time to come up with a good fix. It seems like the way that errors are handled is a little odd. I will take a look at fixing it eventually, but it will require the errors to propagate up rather than just being handled by passing a different sound file. I actually think that would be a good thing. In many situations the current error handling would be poor for human robot interaction. Allowing integrators (like me) to customize how errors are handled would be better. I should get to that at the latest by the middle of February. I'm pretty busy until then.
I'm not sure how to add unit tests, ref #37
@cevans87 @AAlon I fixed the caching of the error file and added a bunch of new tests to get the code coverage up to a reasonable level. I think this is ready for a new review.
Want to check back in on this.