tts-ros1 icon indicating copy to clipboard operation
tts-ros1 copied to clipboard

Caching files

Open mjsobrep opened this issue 5 years ago • 10 comments

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.

mjsobrep avatar Sep 01 '19 03:09 mjsobrep

Codecov Report

Merging #36 (ee320dd) into master (c8ebbf7) will increase coverage by 4.96%. The diff coverage is 94.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

Impacted file tree graph

@@            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.

codecov[bot] avatar Sep 01 '19 14:09 codecov[bot]

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.

mjsobrep avatar Sep 01 '19 15:09 mjsobrep

@AAlon is this something that you all are interested in? Should I keep working on it?

mjsobrep avatar Sep 09 '19 19:09 mjsobrep

@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.

AAlon avatar Sep 13 '19 17:09 AAlon

@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.

mjsobrep avatar Nov 09 '19 22:11 mjsobrep

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.

mjsobrep avatar Nov 30 '19 04:11 mjsobrep

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 avatar Jan 06 '20 21:01 AAlon

@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

mjsobrep avatar Jan 11 '20 17:01 mjsobrep

@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.

mjsobrep avatar Feb 21 '20 01:02 mjsobrep

Want to check back in on this.

mjsobrep avatar Mar 19 '20 19:03 mjsobrep