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

[hailtop.batch] eliminate unittest; use single, session-wide event loop #14097

Merged

Conversation

danking
Copy link
Contributor

@danking danking commented Dec 14, 2023

Fixes #14130.

We pervasively assume:

  1. That our entire system is used within a single Python thread.
  2. That once an event loop is created that's the only event loop that will exist forever.

Pytest (and newer version of IPython, afaict) violate this pretty liberally.

pytest_asyncio has explicit instructions on how to run every test in the same event loop. I've implemented those here. These instructions don't work. It seems that the reliable way to ensure we're using one event loop everywhere is to use pytest-asyncio < 0.23 and to define an event_loop fixture with scope 'session'.

I also switched test_batch.py into pytest-only style. This allows me to use session-scoped fixtures so that they exist exactly once for the entire test suite execution.

Also:

  • RouterAsyncFS methods must either be a static method or an async method. We must not create an FS in a sync method. Both parse_url and copy_part_size now both do not allocate an FS.
  • httpx.py now eagerly errors if the running event loop in request differs from that at allocation time. Annoying but much better error message than this nonsense about timeout context managers.
  • hail_event_loop either gets the current thread's event loop (running or not, doesn't matter to us) or creates a fresh event loop and sets it as the current thread's event loop. The previous code didn't guarantee we'd get an event loop b/c get_event_loop fails if set_event_loop was previously called.
  • conftest.py is inherited downward, so I lifted fixtures out of test_copy.py and friends and into a common hailtop/conftest.py
  • I added make -C hail pytest-inter-cloud for testing the inter cloud directory. You still need appropriate permissions and authn.
  • I removed extraneous pytest.mark.asyncio since we use auto mode everywhere.
  • FailureInjectingClientSession creates an aiohttp.ClientSession and therefore must be used while an event loop is running. Easiest fix was to make the test async.

@danking danking changed the title cleanup [hailtop.batch] eliminate unittest; use single, session-wide event loop Jan 5, 2024
@danking danking force-pushed the cleanup-test-batch-service-backend-tests branch from d0022ef to 3ca8dd8 Compare January 5, 2024 21:46
@danking danking force-pushed the cleanup-test-batch-service-backend-tests branch from 01fa04b to 87b008c Compare January 5, 2024 22:38
@danking danking force-pushed the cleanup-test-batch-service-backend-tests branch from 87b008c to acbf966 Compare January 5, 2024 22:47
@daniel-goldstein
Copy link
Contributor

@danking Looks like we need to upgrade pytest_asyncio

…eate an http session outside of the event loop
@danking danking force-pushed the cleanup-test-batch-service-backend-tests branch from 99fe634 to a36b2bf Compare January 8, 2024 19:39
@daniel-goldstein daniel-goldstein self-assigned this Jan 8, 2024
@danking
Copy link
Contributor Author

danking commented Jan 9, 2024

@daniel-goldstein ok it finally works.

@danking danking merged commit d1823fa into hail-is:main Jan 9, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pytest-asyncio 0.23.* breaks most hail asyncio tests
2 participants