Skip to content

fix: cleanup temporary files#143

Merged
LDeakin merged 3 commits intomainfrom
ld/cleanup_tmp
Feb 2, 2026
Merged

fix: cleanup temporary files#143
LDeakin merged 3 commits intomainfrom
ld/cleanup_tmp

Conversation

@LDeakin
Copy link
Member

@LDeakin LDeakin commented Feb 2, 2026

tempfile.mktemp() does not clean up after itself, and I could see my /tmp growing as I ran a few tests

`tempfile.mktemp()` does not clean up after itself
@LDeakin LDeakin requested a review from ilan-gold February 2, 2026 04:37
@flying-sheep
Copy link
Collaborator

For pytest, you want to use the fixtures for this.

I’ll help

@flying-sheep
Copy link
Collaborator

flying-sheep commented Feb 2, 2026

OK, here we go. Ended up being a major restructuring, since to do this right, we need to override the format and dimensionality fixtures, which means moving the versions of them used in the non-roundtrip tests into conftest. That’ll cause the arr fixture to pick these values from the overridden fixtures, resulting in the exact same set of tests as before.

But now all the scoping is done by pytest, so it cleans up, and the arrays aren’t created in pytests’ collection phase anymore (should speed that up by a lot and therefore help with IDE performance as well)

Copy link
Collaborator

@ilan-gold ilan-gold left a comment

Choose a reason for hiding this comment

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

Awesome paradigm to override, I have found myself in this tricky situation of param dependencies before

@LDeakin LDeakin merged commit 44d5611 into main Feb 2, 2026
17 checks passed
@LDeakin LDeakin deleted the ld/cleanup_tmp branch February 2, 2026 11:49
@flying-sheep
Copy link
Collaborator

Yeah, that’s how pytest is intended to work.

Isaac always wanted flat test file directories (I don’t think I’ve ever heard a reason) so we don’t use it much.

) -> zarr.Array:
return zarr.create(
(axis_size_,) * dimensionality,
store=LocalStore(root=tmp_path / ".zarr"),
Copy link

Choose a reason for hiding this comment

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

would in-memory storage also work here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't think so since this package doesn't support that kind of store crossing into rust

@d-v-b d-v-b mentioned this pull request Feb 2, 2026
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.

4 participants