-
Notifications
You must be signed in to change notification settings - Fork 97
Modifying cleanPath and TempDirChanger behavior
#2402
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
base: main
Are you sure you want to change the base?
Changes from all commits
b9d1243
b4c71ce
b267e41
4073fa6
d11bc73
2b354e1
fb00bf9
737dba4
35d03e8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,16 +26,6 @@ | |
| from armi import context, runLog | ||
| from armi.utils import safeCopy | ||
|
|
||
| DO_NOT_CLEAN_PATHS = [ | ||
| "armiruns", | ||
| "failedruns", | ||
| "mc2run", | ||
| "mongoose", | ||
| "shufflebranches", | ||
| "snapshot", | ||
| "tests", | ||
| ] | ||
|
|
||
|
|
||
| def armiAbsPath(*pathParts): | ||
| """Convert a list of path components to an absolute path, without drive letters if possible.""" | ||
|
|
@@ -186,19 +176,12 @@ def moduleAndAttributeExist(pathAttr): | |
| return moduleAttributeName in userSpecifiedModule.__dict__ | ||
|
|
||
|
|
||
| def cleanPath(path, mpiRank=0): | ||
| """Recursively delete a path. | ||
|
|
||
| !!! Be careful with this !!! It can delete the entire cluster. | ||
|
|
||
| We add copious os.path.exists checks in case an MPI set of things is trying to delete everything | ||
| at the same time. Always check filenames for some special flag when calling this, especially | ||
| with full permissions on the cluster. You could accidentally delete everyone's work with one | ||
| misplaced line! This doesn't ask questions. | ||
| def cleanPath(path, mpiRank=0, tempDir=False): | ||
| """Recursively delete a path. This function checks for a few cases we know to be OK to delete: (1) Any | ||
| `TemporaryDirectoryChanger` instance and (2) anything under the ARMI `_FAST_PATH`. | ||
|
Comment on lines
+180
to
+181
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not what cleanPath originally was meant for. It's worth a discussion. |
||
|
|
||
| Safety nets include an allow-list of paths. | ||
|
|
||
| This makes use of shutil.rmtree and os.remove | ||
| Be careful with editing this! Do not make it a generic can-delete-anything function, because it could in theory | ||
| delete anything a user has write permissions on. | ||
|
|
||
| Returns | ||
| ------- | ||
|
|
@@ -209,25 +192,27 @@ def cleanPath(path, mpiRank=0): | |
| if not os.path.exists(path): | ||
| return True | ||
|
|
||
| for validPath in DO_NOT_CLEAN_PATHS: | ||
| if validPath in path.lower(): | ||
| valid = True | ||
| # Any tempDir can be deleted | ||
| if tempDir: | ||
| valid = True | ||
|
|
||
| if pathlib.Path(context.APP_DATA) in pathlib.Path(path).parents: | ||
| # If the path slated for deletion is a subdirectory of _FAST_PATH, then cool, delete. | ||
| # _FAST_PATH itself gets deleted on program exit. | ||
| if pathlib.Path(path).is_relative_to(pathlib.Path(context.getFastPath())): | ||
| valid = True | ||
|
|
||
| if not valid: | ||
| raise Exception("You tried to delete {0}, but it does not seem safe to do so.".format(path)) | ||
| raise Exception(f"You tried to delete {path}, but it does not seem safe to do so.") | ||
|
|
||
| # delete the file/directory from only one process | ||
| # Delete the file/directory from only one process | ||
| if mpiRank == context.MPI_RANK: | ||
| if os.path.exists(path) and os.path.isdir(path): | ||
| shutil.rmtree(path) | ||
| elif not os.path.isdir(path): | ||
| # it's just a file. Delete it. | ||
| os.remove(path) | ||
|
|
||
| # Potentially, wait for the deletion to finish. | ||
| # Deletions are not immediate, so wait for it to finish. | ||
| maxLoops = 6 | ||
| waitTime = 0.5 | ||
| loopCounter = 0 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,7 +40,7 @@ dependencies = [ | |
| "pluggy>=1.2.0", # Central tool behind the ARMI Plugin system | ||
| "pyDOE>=0.3.8", # We import a Latin-hypercube algorithm to explore a phase space | ||
| "pyevtk>=1.2.0", # Handles binary VTK visualization files | ||
| "ruamel.yaml ; python_version >= '3.11.0'", # Our foundational YAML library | ||
| "ruamel.yaml<0.19.0; python_version >= '3.11.0'", # Our foundational YAML library | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we got one of those "someone released a new package and now everything broke" situations. I didn't look into fixes yet, just testing that this is working first
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Uh oh.
This would be a really unfortunate restriction> BUT, I guess, if you're only changing the version of ruamel.yaml for old versions of Python, then this isn't a game stopped.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually this is for >= 3.11 So it's pretty ugly. :-(
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this was merged into main already, and we will have to address it in the future. I'll make a ticket and resolve this convo about it |
||
| "ruamel.yaml.clib ; python_version >= '3.11.0'", # C-based core of ruamel below | ||
| "ruamel.yaml.clib<=0.2.7 ; python_version < '3.11.0'", # C-based core of ruamel below | ||
| "ruamel.yaml<=0.17.21 ; python_version < '3.11.0'", # Our foundational YAML library | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this makes me think we could call it something else, like safeToDelete
but of course anyone using cleanPath thinks their path is safe to delete!