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

Close open file handle if there aren't loadable_vars #455

Merged
merged 2 commits into from
Mar 2, 2025

Conversation

maxrjones
Copy link
Member

@maxrjones maxrjones commented Mar 1, 2025

There's a ton of warnings like the following when running the tests:

/Users/max/miniforge3/envs/virtualizarr-tests/lib/python3.11/json/encoder.py:357: RuntimeWarning: deallocating CachingFileManager(<class 'h5netcdf.core.File'>, '/private/var/folders/70/hc_nynms54d8lp67z4rsfctc0000gp/T/pytest-of-max/pytest-22/test_cf_fill_value_roundtrip_c5/cf_fill_value.nc', mode='r', kwargs={'invalid_netcdf': None, 'decode_vlen_strings': True, 'driver': None}, manager_id='9996df4a-48c7-4871-b282-9d4619d8f32e'), but file is not already closed. This may indicate a bug.

I think we need to be more careful about open file handles in the readers, this handles one case in which I think it can be closed.

Copy link

codecov bot commented Mar 1, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.58%. Comparing base (bf63593) to head (8534ec7).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #455   +/-   ##
=======================================
  Coverage   88.58%   88.58%           
=======================================
  Files          28       28           
  Lines        1743     1744    +1     
=======================================
+ Hits         1544     1545    +1     
  Misses        199      199           
Files with missing lines Coverage Δ
virtualizarr/readers/common.py 92.72% <100.00%> (+0.13%) ⬆️

@maxrjones maxrjones mentioned this pull request Mar 1, 2025
@chuckwondo
Copy link
Contributor

@maxrjones, see #390

@maxrjones
Copy link
Member Author

@maxrjones, see #390

You called it 😅

Copy link
Member

@TomNicholas TomNicholas left a comment

Choose a reason for hiding this comment

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

Oh nice catch. Yeah the whole loadable_vars implementation is a bit hacky atm, but it will be significantly cleaner after #353 and #427.

@maxrjones
Copy link
Member Author

Oh nice catch. Yeah the whole loadable_vars implementation is a bit hacky atm, but it will be significantly cleaner after #353 and #427.

Unfortunately this didn't actually fix the issue (test is still hanging). Thanks for the pointers to those PRs - good to know that they exist to not spend much time cleaning up any file handling in the existing code.

@TomNicholas TomNicholas merged commit 19caf2f into main Mar 2, 2025
11 checks passed
@TomNicholas TomNicholas deleted the close-filehandle branch March 2, 2025 05:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants