fix flaky transcribe tests by pre-downloading dependencies #11261
Conversation
There was a problem hiding this comment.
The skip is looking good, and the test is quite flaky. However, I will leave the approval to @sannya-singal and @ackdav since I think that this might be the only e2e test for transcribe. So I'll leave it to them if they want to skip it or fix it right away.
|
In this run, It seems the polling is smaller on the next tests after this skipped one, which makes them fail as well. It might be worth it to try to fix the tests? Or we will need to skip all e2e tests. |
There was a problem hiding this comment.
The changes are looking good, but the lock handling should not be necessary in my opinion. 😉
Thanks a lot for jumping on this, and the great team work to really get this stable! The changes are looking good and should ensure that the tests are going to be way more stable in the future! 🚀 💯
| def install_vosk(*args): | ||
| with INIT_VOSK_LOCK: | ||
| if vosk_installed.is_set(): | ||
| return | ||
| try: | ||
| LOG.info("installing Vosk default version") | ||
| vosk_package.install() | ||
| LOG.info("done installing Vosk default version") | ||
| LOG.info( | ||
| "downloading Vosk models used in test: %s", PRE_DOWNLOAD_LANGUAGE_CODE_MODELS | ||
| ) | ||
| for language_code in PRE_DOWNLOAD_LANGUAGE_CODE_MODELS: | ||
| model_name = LANGUAGE_MODELS[language_code] | ||
| # downloading the model takes quite a while sometimes | ||
| TranscribeProvider.download_model(model_name) | ||
| LOG.info( | ||
| "done downloading Vosk model '%s' for language code '%s'", | ||
| model_name, | ||
| language_code, | ||
| ) | ||
| LOG.info("done downloading all Vosk models used in test") | ||
| except Exception: | ||
| LOG.exception("Error during installation of Vosk dependencies") | ||
| installation_errored.set() | ||
| # we also set the other event to quickly stop the polling | ||
| ffmpeg_installed.set() | ||
| finally: | ||
| vosk_installed.set() | ||
|
|
||
| def install_ffmpeg(*args): | ||
| with INIT_FFMPEG_LOCK: | ||
| if ffmpeg_installed.is_set(): | ||
| return | ||
| try: | ||
| LOG.info("installing ffmpeg default version") | ||
| ffmpeg_package.install() | ||
| LOG.info("done ffmpeg default version") | ||
| except Exception: | ||
| LOG.exception("Error during installation of Vosk dependencies") | ||
| installation_errored.set() | ||
| # we also set the other event to quickly stop the polling | ||
| vosk_installed.set() | ||
| finally: | ||
| ffmpeg_installed.set() |
There was a problem hiding this comment.
The locking here shouldn't be necessary in my opinion, because the package installers already handle the locking.
(You could even have fine-grained locking control with the installers, see optional install_lock kwarg on the PackageInstaller class).
There was a problem hiding this comment.
I removed, will wait for the green pipeline and run it once again after just to be sure 👌
| with INIT_VOSK_LOCK: | ||
| if vosk_installed.is_set(): | ||
| return | ||
| try: | ||
| LOG.info("installing Vosk default version") | ||
| vosk_package.install() | ||
| LOG.info("done installing Vosk default version") | ||
| LOG.info( | ||
| "downloading Vosk models used in test: %s", PRE_DOWNLOAD_LANGUAGE_CODE_MODELS | ||
| ) | ||
| for language_code in PRE_DOWNLOAD_LANGUAGE_CODE_MODELS: | ||
| model_name = LANGUAGE_MODELS[language_code] | ||
| # downloading the model takes quite a while sometimes | ||
| TranscribeProvider.download_model(model_name) | ||
| LOG.info( | ||
| "done downloading Vosk model '%s' for language code '%s'", | ||
| model_name, | ||
| language_code, | ||
| ) | ||
| LOG.info("done downloading all Vosk models used in test") | ||
| except Exception: | ||
| LOG.exception("Error during installation of Vosk dependencies") | ||
| installation_errored.set() | ||
| # we also set the other event to quickly stop the polling | ||
| ffmpeg_installed.set() | ||
| finally: | ||
| vosk_installed.set() |
There was a problem hiding this comment.
For another iteration:
This is actually an interesting use case, which could be interesting for other package installers as well.
If you have a service with multiple dependencies, we currently do not really execute them in parallel.
We could implement a native async_install and await_install functions in the PackageInstaller / Package abstraction, which would allow users of multiple package installers to easily implement a fork-join pattern to trigger them in parallel...
But again, something for another time 😛
There was a problem hiding this comment.
Definitely, it's something I thought about as well, we could use a thread pool executor to dispatch all the async_install too, and wait on all the futures returned. Interesting ideas!
There was a problem hiding this comment.
Thanks Ben for an amazing work on pre downloading the dependencies and running them in parallel and already quickly addressing feedback from Alex on the lock handling🚀 Will work on upgrading the packages next and also optimising the installation a bit more 🙌 The changes LGTM!
Motivation
While running the pipeline to check for the release,
test_transcribe_happy_pathflaked again. It is also marked flaky on CircleCI.See failures:
We're using the same logic as in opensearch tests, where a check in the global AWS
conftest.pychecks if there are any tests in the test collection that would match the test parent (test file or test class), and if there is, we already trigger the async download of the dependencies early. When there is the test selection, this does not do a lot, but for full-runs it will help the dependencies being downloaded before the actually tests run.To avoid this issue, I'm using a class fixture pre-downloading the dependencies with a longer timeout, but with a quick failure mode, to avoid waiting on the transcription job if it fails asynchronously
Changes
Testing
Running the pipeline a few times to be sure it's hardened: