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

Wait for threads to exit after stopping model installer/downloader #6014

Merged
merged 6 commits into from Mar 21, 2024

Conversation

lstein
Copy link
Collaborator

@lstein lstein commented Mar 21, 2024

Summary

This PR causes the model_installer and download_queue stop() methods to block until their worker threads have exited. Previously stop() returned immediately.

This fix addresses an issue in the model installer unit test, where the installer is created and torn down in a pytest fixture. Because a new installer was created immediately after the previous installer was torn down, it was possible for the old installer’s threads to intermix with the current ones, causing unpredictable results. This may fix the sporadic hangs and timeouts observed in the model installer unit test.

The downside of this is that the test_model_install and test_download unit tests now run noticeably slower than they used to due to waiting for the threads to exit. The alternative, of scoping the install fixture to the module or session level, is undesirable because we want each test function to start out with a clean install directory and registry.

Related Issues / Discussions

See discord https://discord.com/channels/1020123559063990373/1149513695567810630/1219777315148664882

QA Instructions

If this PR fixes the problem, we will no longer see sporadic timeouts during unit testing. It is a very hard bug to reproduce, however, so time will tell.

Merge Plan

Merge when approved.

Checklist

  • The PR has a short but descriptive title
  • Tests added / updated
  • Documentation added / updated

@github-actions github-actions bot added python PRs that change python files services PRs that change app services python-tests PRs that change python tests labels Mar 21, 2024
@lstein lstein force-pushed the lstein/bugfix/model-install-thread-stop branch from cbe9dd9 to 12f9bda Compare March 21, 2024 02:43
Copy link
Collaborator

@psychedelicious psychedelicious left a comment

Choose a reason for hiding this comment

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

Working fine for me, let's fail forward on this.

I tidied up the log messages a bit.

@psychedelicious psychedelicious enabled auto-merge (rebase) March 21, 2024 05:30
@psychedelicious psychedelicious force-pushed the lstein/bugfix/model-install-thread-stop branch from 276f867 to 78bece9 Compare March 21, 2024 05:31
@psychedelicious psychedelicious merged commit 75f4e27 into main Mar 21, 2024
14 checks passed
@psychedelicious psychedelicious deleted the lstein/bugfix/model-install-thread-stop branch March 21, 2024 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python PRs that change python files python-tests PRs that change python tests services PRs that change app services
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants