Fix file descriptor leak when building global_event_df - #1245
Merged
Merged
Conversation
Each BaseDataset._event_transform starts a dask LocalCluster whose bokeh
dashboard fails to stop on close ("Cannot synchronously wait on a running
event loop"), leaking 3 file descriptors per dataset build. Processes that
build many datasets (the core test suite, benchmark sweeps) exhaust the
macOS default `ulimit -n` of 256 and fail with "OSError: [Errno 24] Too
many open files". Nothing in PyHealth uses the dashboard, so disable it.
- base_dataset: pass dashboard_address=None to LocalCluster
- tests: regression test that fd count stays flat across 4 builds
(fails with 12 leaked fds without the fix)
- docs/install.rst: macOS note with the ulimit workaround for <= 2.0.2
- examples/benchmark_perf/benchmark_workers_n.py: report open_fds per run
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
jhnwu3
approved these changes
Sep 27, 2026
jhnwu3
left a comment
Collaborator
There was a problem hiding this comment.
I will dig deeper into this later. If this fixes the issue with anything else and unblocks you, that's an interesting problem I hadn't considered at the extreme end.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Each
BaseDataset._event_transformstarts a daskLocalCluster. On close, the cluster's bokeh dashboard fails to stop:Each dataset build therefore leaks 3 file descriptors. A process that builds many datasets (the core test suite, or a
benchmark_workers_n.pysweep) exhausts macOS's defaultulimit -nof 256 and fails partway through with:Linux CI doesn't hit this because its default limit is much higher. Every macOS contributor running the suite locally does.
Fix
Pass
dashboard_address=NonetoLocalClusterin_event_transform. Nothing in PyHealth uses the dashboard. This also removes the repeatedPort 8787 is already in usewarnings.Changes
pyhealth/datasets/base_dataset.py: disable the dask dashboard (one line plus a comment).tests/core/test_base_dataset.py: regression test that buildsglobal_event_df4 times and asserts the open-fd count stays flat. Skipped on Windows.docs/install.rst: macOS entry under Platform-Specific Notes, with theulimit -nworkaround for 2.0.2 and earlier.examples/benchmark_perf/benchmark_workers_n.py: recordsopen_fdsafter each run, so the sweep would surface a regression like this.Verification (macOS, Python 3.13)
12 file descriptors leaked across 4 dataset builds) and passes with it.ulimit -n 256:Ran 1328 tests … OK (skipped=76), with 0 "Too many open files" errors. Before the fix the same run failed.tools/check_pr_rules.py --base master --head HEAD: all rules pass.🤖 Generated with Claude Code