Modifying unclear cleanPath behavior

[opotowsky]

What is the change? Why is it being made?

Update: I'm marking this as a feature request instead of a bug for the SCR because although I'm calling it a bug, it wasn't hurting anyone. It's just super unclear behavior we are now correcting.

See #2401 for a full description of the non-ideal behavior.

1. The DO_NOT_CLEAN_PATHS check is removed entirely as it is not needed. It was doing the opposite of what was indicated--it was greenlighting those deletions. We don't need any greenlit deletions other than (2) and (3) below.
2. The APP_DATA check is replaced with a check that the path for deletion is under _FAST_PATH. _FAST_PATH is amended often (e.g. if the code changes directory) and is not guaranteed to be under APP_DATA, but _FAST_PATH and therefore anything in it is always safe to delete.
3. The TemporaryDirectoryChanger root is not guaranteed to be under _FAST_PATH, so an argument is added to indicate to cleanPath the dir slated for deletion comes from a tempDir instance (and this was also applied to output caches as well).

This work exposed some confusion with cleanTempDirs, so we made a ticket for that: #2408. I'm resolving that here because it was found during the course of this work.

SCR Information

Change Type: features

One-Sentence Rationale: cleanPath is not behaving like the variable names would indicate, so issues with naming were corrected in cleanPath as well as in cleanTempDirs.

One-line Impact on Requirements: NA

---

Checklist

• This PR has only one purpose or idea.
• Tests have been added/updated to verify any new/changed code.
• The documentation is still up-to-date in the doc folder.
• The code style follows good practices.
• The dependencies are still up-to-date in pyproject.toml.

[opotowsky]

@john-science this is a feature addition that exposed a bug, so I'm treating it as a bug ticket. I could separate the feature addition into a different PR if you prefer for SCR clarity. The feature is just this:

Also I see a bunch of CI failed. I'll look into that later today....bodes well for my downstream testing 🙃

[john-science]

this is a feature addition that exposed a bug,

Except all changes to this feature to cause massive trauma downstream. People use this feature A LOT, and some people use it in surprising and unexpected ways.

Since I am constantly surprised HOW this is being used, it's hard to know what changes would make this feature more stable.

[john-science]

@opotowsky

One-Sentence Rationale: cleanPath is not behaving properly,

I mean, I don't agree with that. You have need to use this old feature in a new setting it's never been used in before. This is new feature or enhancement at best.

[opotowsky]

@john-science will check on if this can be deleted. Could be cruft from the old days.

Does shutil only return once it's complete? If yes, can delete

[john-science]

I can confirm:

• This is cruft from the ancient past.
• Their docs do not say that shutil.rmtree() is blocking. But digging around the code for a while convinces me it is.

[john-science]

I love your re-design of the PR description. Thank you!

[john-science]

I'm 50/50 on this. Should this new parameter be called tempDir, or something like force?

[opotowsky]

Oh yea, I'm very willing to go over ideas here (see comment reply here #2402 (comment))

Really, as reviewer you can tell me what to do here.

force
forceDelete
forceClean
allowDelete
yesIHaveAPermit
AUTHORIZED
clearedForDeletion
blessedBeTheDeletion
outputCacheOrTempDirDeletionONLY
hallPassForDestruction

ok wow I need to stop

[john-science]

I would go with force or forceClean, if I was picking from your list. #shrug