From 60d4641b2aa118cd2e8f5f5757363fd7169b43e1 Mon Sep 17 00:00:00 2001 From: Xylar Asay-Davis Date: Fri, 23 Oct 2026 20:40:38 +0200 Subject: [PATCH] Remove temporary directories in cull() and convert() Both functions created a working directory with mkdtemp() and never removed it, leaving copies of the input mesh, masks and output mesh behind on every call, despite their docstrings saying temporary files are deleted. Use TemporaryDirectory, as mask() already does, closing the loaded output dataset and copying any graph file before the directory is removed. Add a test that the working directory only contains the requested graph files after culling and converting a mesh. Co-Authored-By: Claude Opus 5.5 (1M context) --- conda_package/mpas_tools/mesh/conversion.py | 106 ++++++++++---------- conda_package/tests/test_conversion.py | 25 +++++ 2 files changed, 77 insertions(+), 54 deletions(-) diff --git a/conda_package/mpas_tools/mesh/conversion.py b/conda_package/mpas_tools/mesh/conversion.py index e8c890872..f66071a8f 100644 --- a/conda_package/mpas_tools/mesh/conversion.py +++ b/conda_package/mpas_tools/mesh/conversion.py @@ -1,6 +1,6 @@ import os import shutil -from tempfile import TemporaryDirectory, mkdtemp +from tempfile import TemporaryDirectory import numpy as np import xarray as xr @@ -47,24 +47,23 @@ def convert(dsIn, graphInfoFileName=None, logger=None, dir=None): if dir is not None: dir = os.path.abspath(dir) - tempdir = mkdtemp(dir=dir) - inFileName = f'{tempdir}/mesh_in.nc' - write_netcdf(dsIn, inFileName) - - outFileName = f'{tempdir}/mesh_out.nc' - if graphInfoFileName is not None: graphInfoFileName = os.path.abspath(graphInfoFileName) - outDir = os.path.dirname(outFileName) + with TemporaryDirectory(dir=dir) as tempdir: + inFileName = f'{tempdir}/mesh_in.nc' + write_netcdf(dsIn, inFileName) - check_call(['MpasMeshConverter.x', inFileName, outFileName], logger) + outFileName = f'{tempdir}/mesh_out.nc' - dsOut = xr.open_dataset(outFileName) - dsOut.load() + check_call(['MpasMeshConverter.x', inFileName, outFileName], logger) - if graphInfoFileName is not None: - shutil.copyfile(f'{outDir}/graph.info', graphInfoFileName) + dsOut = xr.open_dataset(outFileName) + dsOut.load() + dsOut.close() + + if graphInfoFileName is not None: + shutil.copyfile(f'{tempdir}/graph.info', graphInfoFileName) return dsOut @@ -121,53 +120,52 @@ def cull( """ if dir is not None: dir = os.path.abspath(dir) - tempdir = mkdtemp(dir=dir) - inFileName = f'{tempdir}/ds_in.nc' - dsIn = _masks_to_int(dsIn) - write_netcdf(dsIn, inFileName) - outFileName = f'{tempdir}/ds_out.nc' - - args = ['MpasCellCuller.x', inFileName, outFileName] - - if dsMask is not None: - if not isinstance(dsMask, list): - dsMask = [dsMask] - for index, ds in enumerate(dsMask): - ds = _masks_to_int(ds) - fileName = f'{tempdir}/mask{index}.nc' - write_netcdf(ds, fileName) - args.extend(['-m', fileName]) - - if dsInverse is not None: - if not isinstance(dsInverse, list): - dsInverse = [dsInverse] - for index, ds in enumerate(dsInverse): - ds = _masks_to_int(ds) - fileName = f'{tempdir}/inverse{index}.nc' - write_netcdf(ds, fileName) - args.extend(['-i', fileName]) - - if dsPreserve is not None: - if not isinstance(dsPreserve, list): - dsPreserve = [dsPreserve] - for index, ds in enumerate(dsPreserve): - ds = _masks_to_int(ds) - fileName = f'{tempdir}/preserve{index}.nc' - write_netcdf(ds, fileName) - args.extend(['-p', fileName]) - if graphInfoFileName is not None: graphInfoFileName = os.path.abspath(graphInfoFileName) - outDir = os.path.dirname(outFileName) + with TemporaryDirectory(dir=dir) as tempdir: + inFileName = f'{tempdir}/ds_in.nc' + dsIn = _masks_to_int(dsIn) + write_netcdf(dsIn, inFileName) + outFileName = f'{tempdir}/ds_out.nc' + + args = ['MpasCellCuller.x', inFileName, outFileName] + + if dsMask is not None: + if not isinstance(dsMask, list): + dsMask = [dsMask] + for index, ds in enumerate(dsMask): + ds = _masks_to_int(ds) + fileName = f'{tempdir}/mask{index}.nc' + write_netcdf(ds, fileName) + args.extend(['-m', fileName]) + + if dsInverse is not None: + if not isinstance(dsInverse, list): + dsInverse = [dsInverse] + for index, ds in enumerate(dsInverse): + ds = _masks_to_int(ds) + fileName = f'{tempdir}/inverse{index}.nc' + write_netcdf(ds, fileName) + args.extend(['-i', fileName]) + + if dsPreserve is not None: + if not isinstance(dsPreserve, list): + dsPreserve = [dsPreserve] + for index, ds in enumerate(dsPreserve): + ds = _masks_to_int(ds) + fileName = f'{tempdir}/preserve{index}.nc' + write_netcdf(ds, fileName) + args.extend(['-p', fileName]) - check_call(args=args, logger=logger) + check_call(args=args, logger=logger) - dsOut = xr.open_dataset(outFileName) - dsOut.load() + dsOut = xr.open_dataset(outFileName) + dsOut.load() + dsOut.close() - if graphInfoFileName is not None: - shutil.copyfile(f'{outDir}/culled_graph.info', graphInfoFileName) + if graphInfoFileName is not None: + shutil.copyfile(f'{tempdir}/culled_graph.info', graphInfoFileName) return dsOut diff --git a/conda_package/tests/test_conversion.py b/conda_package/tests/test_conversion.py index 23377d622..2ad79c089 100755 --- a/conda_package/tests/test_conversion.py +++ b/conda_package/tests/test_conversion.py @@ -6,6 +6,7 @@ from mpas_tools.io import write_netcdf from mpas_tools.mesh.conversion import _masks_to_int, convert, cull, mask from mpas_tools.mesh.spherical import recompute_angle_edge +from mpas_tools.planar_hex import make_planar_hex_mesh from .util import get_test_data_file @@ -82,6 +83,30 @@ def test_conversion_triangle_angle_quality(): assert np.array_equal(ds_mesh.obtuseTriangle.values, obtuse) +def test_cull_convert_remove_temp_dir(tmp_path): + ds_mesh = make_planar_hex_mesh( + nx=10, ny=10, dc=1e3, nonperiodic_x=True, nonperiodic_y=True + ) + ds_culled = cull( + ds_mesh, + graphInfoFileName=str(tmp_path / 'culled_graph.info'), + dir=tmp_path, + ) + ds_converted = convert( + ds_culled, graphInfoFileName=str(tmp_path / 'graph.info'), dir=tmp_path + ) + + # only the graph files should remain, and the datasets should still be + # usable after their temporary files are deleted + assert sorted(path.name for path in tmp_path.iterdir()) == [ + 'culled_graph.info', + 'graph.info', + ] + assert ds_culled.sizes['nCells'] == 100 + assert ds_converted.sizes['nCells'] == 100 + assert np.all(ds_converted.areaCell.values > 0.0) + + def test_masks_to_int_dataset_copy(): ds_in = xarray.Dataset( data_vars={