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

Create a single tmp dir per test session #20

Merged
merged 5 commits into from
Apr 3, 2024
Merged

Conversation

delucchi-cmu
Copy link
Contributor

@delucchi-cmu delucchi-cmu commented Apr 3, 2024

Previous behavior was to create a new tmp directory for each test case that required writing to the cloud bucket. That adds a lot of overhead, as removing the directory is a slow, asynchronous call. We have an exponential backoff and retry to delete, but there were around 600 temp directories that were not cleaned up due to failed attempts.

This change creates a single temp directory in the cloud bucket for the whole execution, and cleans it up at the end of the testing session. I increase the number of retries, to get one more chance to successfully delete before giving up.

Because we only have ONE exponential backoff/retry cycle for the whole test session, we trim about 15-30 seconds off the total execution time

In addition, since this changed so many test files, I figured it was an ok time to rename the overly-long example_cloud_storage_options fixture. Also cleans up the session-level fixtures.

Copy link

github-actions bot commented Apr 3, 2024

Before [153c401] After [189db0b] Ratio Benchmark (Parameter)
2.66±0.9s 881±1000ms ~0.33 benchmarks.time_computation
2.06k 3.52k 1.71 benchmarks.mem_list

Click here to view all benchmarks.

Copy link

@jeremykubica jeremykubica left a comment

Choose a reason for hiding this comment

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

Looks like a nice change in terms of efficiency. I left two questions for my own understanding.

tests/conftest.py Outdated Show resolved Hide resolved
tests/conftest.py Show resolved Hide resolved
@delucchi-cmu delucchi-cmu merged commit 5480214 into main Apr 3, 2024
7 checks passed
@delucchi-cmu delucchi-cmu deleted the delucchi/tmp_path branch April 3, 2024 20:16
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.

2 participants