In astropy/utils/data.py, update _rmtree to use shutil.move instead of os.rename.
#13730
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to Astropy 👋 and congratulations on your first pull request! 🎉
A project member will respond to you as soon as possible; in the meantime, please have a look over the Checklist for Contributed Code and make sure you've addressed as many of the questions there as possible.
If you feel that this pull request has not been responded to in a timely manner, please leave a comment mentioning our software support engineer @embray, or send a message directly to the development mailing list. If the issue is urgent or sensitive in nature (e.g., a security vulnerability) please send an e-mail directly to the private e-mail feedback@astropy.org.
pllim
added
Bug
💤 backport-v5.0.x
|
Alas, the failure looks related. Since @aarchiba wrote the original code and test here, maybe she can have a look as well. |
|
If you decide to pursue this fix, please also add a change log. Thanks! See https://github.com/astropy/astropy/blob/main/docs/changes/README.rst |
|
Since there is failure that needs addressing, I am converting this to draft. FYI. |
|
Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then. In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary. If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock. If you believe I commented on this pull request incorrectly, please report this here. |
tribeiro
force-pushed
the
replace-os-rename-with-shutil-move
branch
9 times, most recently
from
6119b5c
to
b090c31
Compare
|
@pllim , I changed the implementation to keep the original I also updated the changelog. |
tribeiro
force-pushed
the
replace-os-rename-with-shutil-move
branch
from
b090c31
to
ca38f95
Compare
docs/changes/utils/13730.bugfix.rst
Outdated
| @@ -0,0 +1 @@ | |||
| Update ``_rmtree`` to catch ``OSError`` and use ``shutil.move`` if errno == EXDEV. | |||
There was a problem hiding this comment.
Since _rmtree is "hidden", it should not be explicitly mentioned like this in a change log. But rather, what do you want the end-user to know about this patch? How is the data.py behavior changing for them at a high level?
There was a problem hiding this comment.
Changelog updated.
|
@aarchiba , are you interested to review? Thanks, everyone! |
|
Re: coverage -- Codecov does say your patch is 100% covered (https://app.codecov.io/gh/astropy/astropy/pull/13730). However, it is complaining about "indirect changes" at the project level; I am not clear why either. Have you rebased against |
I did rebased it.. |
tribeiro
force-pushed
the
replace-os-rename-with-shutil-move
branch
2 times, most recently
from
b2e5b7d
to
c902bfb
Compare
This comment was marked as resolved.
This comment was marked as resolved.
astropy/utils/tests/test_data.py
Outdated
| with open(filename, "w") as f: | ||
| f.write(content) | ||
|
|
||
| with pytest.warns(AstropyWarning): |
There was a problem hiding this comment.
Should also use match= to check the warning message here.
astropy/utils/tests/test_data.py
Outdated
| monkeypatch.setattr(os, "rename", no_rename) | ||
|
|
||
| assert is_url_in_cache(u) | ||
| with pytest.warns(AstropyWarning): |
There was a problem hiding this comment.
Should also use match= to check the warning message here.
| if e.errno == errno.EXDEV: | ||
| warn(e.strerror, AstropyWarning) | ||
| shutil.move(path, os.path.join(d, "to-zap")) | ||
| else: |
There was a problem hiding this comment.
Now I see that the original code did not raise on OSError but perhaps it was an oversight. I do not see why it should be ignored.
…`shutil.move`` if errno == EXDEV. Using `os.rename` may raise an "OSError: [Errno 18] Invalid cross-device link" exception when running o a containairized environments deployed on kubernetes.
…or when ``os.rename`` raises an OSError exception with ``errno = errno.EXDEV``.
tribeiro
force-pushed
the
replace-os-rename-with-shutil-move
branch
from
2a4d864
to
e5afb33
Compare
|
Hi @pllim , I implemented all your suggestions and rebased to the branch you recommended. It seems like all the checks are passing now (though there's one still running at the time of this writing). |
|
Hi. Just want to let you know this is on my queue to look at. Thanks for your patience! |
Using
os.renamemay raise anexception when running on a containairized environments deployed on kubernetes.
Description
This pull request is to address an error we get when using astropy in a containerized environment deployed using Kubernetes. For some reason using
os.renameon these environments raises anOSErrorexception even when the file is in the same directory. Replacingos.renamewithshuttle.movesolves this issue.Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
Extra CIlabel. Codestyle issues can be fixed by the bot.no-changelog-entry-neededlabel. If this is a manual backport, use theskip-changelog-checkslabel unless special changelog handling is necessary.astropy-botcheck might be missing; do not let the green checkmark fool you.backport-X.Y.xlabel(s) before merge.