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

Remove silent overwriting of shutil.copy in ARMI utils #1690

Open
opotowsky opened this issue Apr 18, 2024 · 3 comments · Fixed by #1691
Open

Remove silent overwriting of shutil.copy in ARMI utils #1690

opotowsky opened this issue Apr 18, 2024 · 3 comments · Fixed by #1691
Assignees
Labels
enhancement New feature or request

Comments

@opotowsky
Copy link
Member

safeCopy silently overwrites shutil.copy in ARMI:

armi/armi/utils/__init__.py

Lines 807 to 825 in 08f8c1d

def safeCopy(src: str, dst: str) -> None:
"""This copy overwrites ``shutil.copy`` and checks that copy operation is truly completed before continuing."""
waitTime = 0.01 # 10 ms
if os.path.isdir(dst):
dst = os.path.join(dst, os.path.basename(src))
srcSize = os.path.getsize(src)
shutil.copyfile(src, dst)
shutil.copymode(src, dst)
while True:
dstSize = os.path.getsize(dst)
if srcSize == dstSize:
break
time.sleep(waitTime)
runLog.extra("Copied {} -> {}".format(src, dst))
# Allow us to check the copy operation is complete before continuing
shutil_copy = shutil.copy
shutil.copy = safeCopy

I'm making this ticket so that when shutil.copy is replaced in all downstream plugins with safeCopy, I want to remove this menace from the code base.

I also need to check in with other ARMI users so they have a heads up on a potentially breaking change (so, if shutil is being leveraged, they can do nothing and revert to the real shutil.copy, or they can also swap to explicitly using safeCopy)

@john-science john-science added the enhancement New feature or request label Apr 19, 2024
@john-science
Copy link
Member

I agree, silently over-writing a standard library tool whenever you import armi is a bad solution to whatever problem this was trying to solve.

I would say "let's do it right now!", but since it's an API-breaking change, we will be slow and deliberate.

@drewj-usnctech Just FYI.

@john-science john-science linked a pull request Apr 19, 2024 that will close this issue
7 tasks
@onufer
Copy link
Contributor

onufer commented May 15, 2024

this seems super low priority. I agree it is confusing and surprising but its going to be a lot of work to change and very little benifit.

@onufer onufer reopened this May 15, 2024
@opotowsky
Copy link
Member Author

Thanks for reopening @onufer

I grepped around and it won't be a huge lift. I agree it's low priority, but I'd like to get in the habit of paying back some tech debt and this is an easy one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants