feat: testing and cleanup #14

mikejgray · 2024-01-01T07:06:58Z

features

100% unit test coverage
some refactors to accommodate testing
e2e tests (76% coverage where they're passing)
GHA workflows to do all testing and linting
- bandit
- safety
- flake8
- isort
- ruff
- mypy

Any TTS plugin with a Docker image running the server can be used in an e2e test by updating the test matrix.

Closes #13

builderjer · 2024-01-01T13:00:20Z

I get the mimic binaries here
https://github.com/OpenVoiceOS/raspOVOS/tree/dev/stage-prep/00-configure-apt
its a custom repo from mycroft days. Only for ubuntu flavors though

builderjer

Couple of comments, or questions I guess. And I am still learning the tests and automatons. Maybe this will be a good template for me.

builderjer · 2024-01-01T12:50:23Z

ovos_tts_plugin_server/__init__.py

-        super().__init__(*args, **kwargs, audio_ext="wav",
-                         validator=OVOSServerTTSValidator(self))
+        super().__init__(*args, **kwargs, audio_ext="wav", validator=OVOSServerTTSValidator(self))
+        self.log = LOG(name=__name__)


Can you not use just LOG.debug here instead of defining the self.log?
I think it automatically adds the name of the plugin/skill being logged?

Not that it matters I don't think, but for clarity, which one is more appropriate?

Not sure, I'd lean on @JarbasAl for insight

to add to the questionaire, how (or where) does the LOG singleton get the parent module (plugin wise) from? ie. ovos-tts-server-plugin -> audio

In my estimation this creates a log line ... - ovos-tts-server-plugin - x:x:1 - ... (instead of ... - audio - ...) in the ovos.log (instead audio.log)

Here's the canonical boilerplate. I haven't touched this in so long, I'm not sure I can speak with any authority to what can be removed, but I'd expect the initialization of the logger itself to be the only necessary action on your part.

https://github.com/OpenVoiceOS/ovos-core/blob/228835b72f4e296b862e01291cb51c150ae421ef/ovos_core/__init__.py#L15-L23

edit: I expected that to embed the lines

from ovos_config.config import Configuration from ovos_utils.log import LOG _cfg = Configuration() _log_level = _cfg.get("log_level", "INFO") _logs_conf = _cfg.get("logs") or {} _logs_conf["level"] = _log_level LOG.init(_logs_conf) # read log level from config

Looks like the way OVOSSkill implements it is to set self.log = LOG (not instantiate it) and then run LOG.create_logger(skill_id) (https://github.com/OpenVoiceOS/OVOS-workshop/blob/202a3eb59f01d64c35fbf46dfaa55c581dd56dce/ovos_workshop/skills/ovos.py#L714)

All of the templates in ovos-plugin-manager simply import LOG and use that, however. What's the reasoning behind one vs the other? I also saw that while the TTS class uses LOG, STT has no logging capabilities rolled into the base class.

The boilerplate in Core is configuring what should be a namespace- or application-level singleton. The boilerplate in accessory modules therefore doesn't need to configure the logger. It simply imports the same logger Core is using, and then uses it.

To my embarrassment, I'm just noticing this is the TTS server plugin, not tts-server. You should be able to ape the templates. @JarbasAl can confirm or deny, but if nothing has changed, that's what I'd do.

What i'm getting at, unless you want to change the default behaviour and dump an ungrouped log in ovos.log, there is most likely no need to init an new logger. (This is done in the service modules)

The question remains, where is this group set?

Switching to the established plugin pattern unless I'm told otherwise. Thanks for the context, all!

@JarbasAl Looking for an answer to @emphasize 's questions above, don't think we have an answer for him

ovos_tts_plugin_server/__init__.py

builderjer · 2024-01-01T13:20:48Z

ovos_tts_plugin_server/__init__.py

+            f.write(data)
+
+    def _fetch_audio_data(self, params: dict, sentence: str, servers: list) -> bytes:
+        """Get audio bytes from servers."""
        random.shuffle(servers)  # Spread the load among all public servers


It was mentioned with the STT server that only the public servers should be shuffled. Does this shuffle the list no matter where it came from?

Yes, it does. That's the original behavior, I didn't change anything

mikejgray · 2024-01-01T13:45:40Z

These tests and automations are very custom for this repo, not sure how useful it'd be as a template, but it does have 100% unit test coverage and 70+% on the e2e tests. I can probably hit 100% on those too once I get them all working.

The unit test workflow file I'm considering pushing to OpenVoiceOS/.github as a shared workflow if people like it. It's very comprehensive and opinionated, so repos would need significant work to pass that workflow.

features - significant unit test coverage - some refactors to accommodate testing - e2e tests - GHA workflows to do all testing and linting - bandit - safety - flake8 - isort - ruff - mypy fixes for ci/cd more fixes test fixes more test fixes sudo more e2e more fixes further improvements improve e2e testing get better logs give it some time, and drop testing for mimic3 get mimic running for e2e testing sudo it more e2e attempted fixes another approach for e2e fixes better logic more special sam work, force v1 sam is very special more attempted fixes another approach branch piper tests

remove now unnecessary mimic stuff

emphasize · 2024-01-01T22:11:58Z

ovos_tts_plugin_server/__init__.py

-                    url = f'{url}/synthesize/{sentence}'
-                r = requests.get(url, params=params, verify=self.verify_ssl)
+                    url = f"{url}/synthesize/{sentence}"
+                r: requests.Response = requests.get(url=url, params=params, verify=self.verify_ssl, timeout=30)


out of scope here, but since you touched it: shorter timeout?

I thought about that, opted for 30 in case someone is using TTS with a very high RTF, even though 10 is probably already too long. It should probably be configurable - maybe an issue for a next PR?

I touched this at all to satisfy a linter btw

accurately we would want to set a timeout based on the input data size and assume a conversion factor

I'm ok with the 30, as long as we get back to it on a later date. (same on STT, btw)
Plus, this is a public community server that shouldn't be fed with overly long input.
(Brainfart: we should default it to 30 and put it behind a config value - and change it later to be more accurate)

builderjer reviewed Jan 1, 2024

View reviewed changes

mikejgray force-pushed the FEAT_TestingAndLinting branch from 4b8ec4a to 72b3226 Compare January 1, 2024 17:25

mikejgray added 2 commits January 1, 2024 15:09

follow established plugin logging pattern

d63ceb9

remove now unnecessary mimic stuff

fix logging test

c92df13

mikejgray requested review from builderjer, emphasize and ChanceNCounter January 1, 2024 21:16

emphasize reviewed Jan 1, 2024

View reviewed changes

emphasize approved these changes Jan 1, 2024

View reviewed changes

This was referenced Jan 2, 2024

configurable timeout #15

Open

only shuffle public servers #16

Open

JarbasAl merged commit 42509fe into OpenVoiceOS:dev Jan 2, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: testing and cleanup #14

feat: testing and cleanup #14

mikejgray commented Jan 1, 2024 •

edited

builderjer commented Jan 1, 2024

builderjer left a comment

builderjer Jan 1, 2024

mikejgray Jan 1, 2024

emphasize Jan 1, 2024 •

edited

ChanceNCounter Jan 1, 2024 •

edited

mikejgray Jan 1, 2024

ChanceNCounter Jan 1, 2024

emphasize Jan 1, 2024 •

edited

mikejgray Jan 1, 2024

mikejgray Jan 1, 2024

builderjer Jan 1, 2024

mikejgray Jan 1, 2024

mikejgray commented Jan 1, 2024

emphasize Jan 1, 2024

mikejgray Jan 1, 2024

mikejgray Jan 1, 2024 •

edited

emphasize Jan 1, 2024

emphasize Jan 1, 2024 •

edited

feat: testing and cleanup #14

feat: testing and cleanup #14

Conversation

mikejgray commented Jan 1, 2024 • edited

builderjer commented Jan 1, 2024

builderjer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emphasize Jan 1, 2024 • edited

Choose a reason for hiding this comment

ChanceNCounter Jan 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emphasize Jan 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikejgray commented Jan 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikejgray Jan 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emphasize Jan 1, 2024 • edited

Choose a reason for hiding this comment

mikejgray commented Jan 1, 2024 •

edited

emphasize Jan 1, 2024 •

edited

ChanceNCounter Jan 1, 2024 •

edited

emphasize Jan 1, 2024 •

edited

mikejgray Jan 1, 2024 •

edited

emphasize Jan 1, 2024 •

edited