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

Deprecation fix the 2nd: replace mktemp with TemporaryDirectory #796

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dhomeier
Copy link
Contributor

Another go at replacing the deprecated function for the save_to_tmp_dir option, with an attempt to delete the temporary files along with the cube instance.
See however discussion about using __del__ in #789 (review).

@codecov-commenter
Copy link

Codecov Report

Merging #796 (4b72221) into master (c7281a7) will decrease coverage by 0.00%.
The diff coverage is 37.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #796      +/-   ##
==========================================
- Coverage   78.00%   78.00%   -0.01%     
==========================================
  Files          24       24              
  Lines        5847     5850       +3     
==========================================
+ Hits         4561     4563       +2     
- Misses       1286     1287       +1     
Impacted Files Coverage Δ
spectral_cube/dask_spectral_cube.py 85.54% <37.50%> (-0.10%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7281a7...4b72221. Read the comment docs.

@dhomeier
Copy link
Contributor Author

Apparently the Windows test is still the only one that runs with zarr installed and not skipping the tests. Don't quite understand why, as it's also listed in the novis deps.

Copy link
Contributor

@keflavich keflavich left a comment

Choose a reason for hiding this comment

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

LGTM, but I think we should hold off on merging for a minute - I'd like to do some additional tests.

In particular, I plan to by-hand load a cube, do some save_to_tmpdir operation, verify that the zarr is written to disk, delete the cube, verify that the tmpdir has been deleted. Similarly, same process, but close python w/o deleting tempdir.

Also we should probably figure out the test issue that @dhomeier noted - we should test this on each OS

@dhomeier
Copy link
Contributor Author

Similarly, same process, but close python w/o deleting tempdir.

Good point, something I've also only tested locally under macOS. Not sure if this can be implemented in the CI – I think open_files is kind of testing this, but may not be completely equivalent. And I have no idea about tmpfile handling on Windows in general.

@keflavich
Copy link
Contributor

@dhomeier could you rebase this? I think we're ready to merge even though I never did end up doing that manual test

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