tests: garbage collect sqlite connections on Windows + Python 3.11 #8547
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
skshetry
force-pushed
the
test-windows
branch
from
0e0afb2 to
5e3445f
Compare
Codecov ReportBase: 94.31% // Head: 45.65% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #8547 +/- ##
===========================================
- Coverage 94.31% 45.65% -48.67%
===========================================
Files 430 430
Lines 32830 32840 +10
Branches 4581 4585 +4
===========================================
- Hits 30963 14992 -15971
- Misses 1449 17287 +15838
- Partials 418 561 +143
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
skshetry
force-pushed
the
test-windows
branch
from
150bd35 to
4c3f8c9
Compare
skshetry
changed the title
There seems to be a regression in Python 3.11, where the sqlite connections are not deallocated, due to some internal changes in Python 3.11, where they are now using LRU cache. They are not deallocated until `gc.collect()` is not called. See python/cpython#97641. This affects only Windows, because when we try to remove the tempdir for the exp run, the sqlite connection is open which prevents us from deleting that folder. Although this may happen in real scenario in `exp run`, I am only fixing the tests by mocking `dvc.close()` and extending it by calling `gc.collect()` after it. We could also mock `State.close()` but didnot want to mock something that is not in dvc itself. The `diskcache` uses threadlocal for connections, so they are expected to be garbage collected, and therefore does not provide a good way to close the connections. The only API it offers is `self.close()` and that only closes main thread's connection. If we had access to connection, an easier way would have been to explicitly call `conn.close()`. But we don''t have such option at the moment. Related: iterative#8404 (comment) GHA Failure: https://github.com/iterative/dvc/actions/runs/3437324559/jobs/5731929385#step:5:57
skshetry
force-pushed
the
test-windows
branch
from
4c3f8c9 to
288a23a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
Seems like this will be required until the underlying python bug is fixed (windows tests are still flaky) https://github.com/iterative/dvc/actions/runs/3442517841/jobs/5743178770 |
|
@pmrowla, what do you think of skipping deletion of temporary workspace on failures? |
We've considered that in the past, but the end result was that users would just generate a ton of (failed) temp workspace copies without realizing they needed to run some additional command to clean them up afterwards. |
There seems to be a regression in Python 3.11, where the sqlite
connections are not deallocated, due to some internal changes in
Python 3.11, where they are now using LRU cache. They are not deallocated
until
gc.collect()is called.See python/cpython#97641.
This affects only Windows, because when we try to remove the tempdir for
the exp run, the sqlite connection is open which prevents us from
deleting that folder.
Although this may happen in real scenario in
exp run, I am only fixingthe tests by mocking
dvc.close()and extending it by callinggc.collect()after it. We could also mockState.close()but didnotwant to mock something that is not in dvc itself.
The
diskcacheuses threadlocal for connections, so they are expectedto be garbage collected, and therefore does not provide a good way to
close the connections. The only API it offers is
self.close()and thatonly closes main thread's connection. If we had access to connection,
an easier way would have been to explicitly call
conn.close().But we don''t have such option at the moment.
Related: #8404 (comment)
GHA Failure: https://github.com/iterative/dvc/actions/runs/3437324559/jobs/5731929385#step:5:57