Deprecate delete_removed_files from ScratchDir

[DanielYang59]

• Change delete_removed_files of ScratchDir default to False, to fix ScratchDir("test_dir", copy_to_current_on_exit=True) would wipe current working directory #760

@Andrew-S-Rosen @shyuep I'm not sure what's the best fix here? Maybe we should remove the argument altogether (not sure if there's any use case for this)?

I assume from a user's prospective one would expect to run a job in a scratch directory, and decide whether or not to copy newly generated files to CWD, in any case I wouldn't expect CWD to be wiped?

[codecov]

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.11%. Comparing base (a0a2265) to head (de9bd37).

Files with missing lines	Patch %	Lines
src/monty/tempfile.py	0.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #762      +/-   ##
==========================================
- Coverage   85.27%   85.11%   -0.17%     
==========================================
  Files          27       27              
  Lines        1664     1659       -5     
  Branches      315      314       -1     
==========================================
- Hits         1419     1412       -7     
- Misses        186      187       +1     
- Partials       59       60       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Andrew-S-Rosen]

If you are removing functionality altogether, then we should be removing the keyword argument. I am not opposed to its removal because I am generally of the opinion that packages should not delete files unless absolutely necessary --- too much room for disaster.

[DanielYang59]

Thanks for the comment, I guess you're suggesting that we just remove the keyword argument directly without deprecation warning (therefore would be breaking)? I'm personally not sure if it's worth it because currently that argument is effectively doing nothing and only issue a warning such that users would have time to react

[Andrew-S-Rosen]

Isn't it currently breaking as the features for the kwarg were removed?

[DanielYang59]

emmm I don't think currently it would be breaking? i.e. if someone still try to use after we remove the features:

with ScratchDir(delete_removed_files=True):

It would just be no-op and issue a deprecation warning.

But if we remove delete_removed_files directly it would error out with unexpected keyword argument?

[Andrew-S-Rosen]

This is where I was confused. It looked like this would change the behavior and therefore be a breaking change anyway. My feeling was that if we are already introducing a breaking change and the argument now does literally nothing but raise a warning, then we should just be removing it altogether. We either keep the old behavior with a deprecation warning or we remove altogether, but changing the behavior and adding a deprecation warning does not make sense to me. Perhaps I have misunderstood.

[DanielYang59]

It looked like this would change the behavior and therefore be a breaking change anyway.

I understand your point now, so you were suggesting that making the delete_removed_files a no-op would be a "breaking change" as it changes the behaviour (it indeed does). But I'm not sure if this is really the case here as from my understand:

1. It doesn't change the interface (delete_removed_files is still there for issuing the warning)
2. The core functionality of ScratchDir is to create a scratch dir and delete_removed_files is for an optional clean up of the CWD (not the temp dir), so it would only be a breaking change if user really relies on the deletion behaviour (again which I consider not the core functionality). As I understand it, the core functionality is desribed below (i.e. create temp dir -> user operates in temp dir -> remove temp dir): https://github.com/materialsvirtuallab/monty/blob/a0a22650dfea5e13848ac8dc26a9da7a32301958/src/monty/tempfile.py#L36-L42

---

I pasted the question to GPT and here is what I got in case it's helpful for the discussion:

[Andrew-S-Rosen]

This is mostly semantics at this point, but if delete_removed_files used to delete removed files and now it doesn't, that's a breaking change even if it can have a dangerous side effect. The behavior is changed, and someone could have been relying on that. I disagree with ChatGPT.

This is just my take. And in the end, I personally do not really care what happens here to be quite honest. As long as the dangerous behavior is gone, I'm fine with that. I do not have reservations with this being merged.

[DanielYang59]

if delete_removed_files used to delete removed files and now it doesn't, that's a breaking change

I guess current behaviour is confusing/implicit in the first place, as it implicitly depends on copy_to_current_on_exit. Currently behaviour seems to be:

So I'm not sure if there is such case as "delete_removed_files used to delete removed files and now it doesn't" as when copy_to_current_on_exit=True, it would either nuke CWD (expected?) or do nothing (unexpected) ...

I also don't have a strong opinion to this end (the priority would be fixing the dangerous behaviour, which we did here), and I agree the rest is semantics. So I would let you decide.

[DanielYang59]

I just searched across GitHub and didn't find any usage of delete_removed_files=True at all, guess we just remove it

Also: https://sourcegraph.com/search?q=context%3Aglobal+delete_removed_files&patternType=keyword&sm=0&df=%5B%22lang%22%2C%22Python%22%2C%22lang%3Apython%22%5D&__cc=1

[elemynt-ong]

Keep the kwarg and raise a Deprecation Warning and inform user htat this arg does nothing and will be removed in 2027.

[DanielYang59]

@shyuep @elemynt-ong I think this should be ready for review, thanks