Handle PermissionError in os.chmod Call - Update engine.py #4139
Conversation
This branch includes changes to handle potential exceptions that may occur when attempting to change file permissions using the os.chmod function within the DeepSpeed engine. The specific issue addressed is the PermissionError that may arise when working with certain filesystems or under restricted permissions.
M-Chris
changed the title
|
@M-Chris, thanks for this PR. Is it possible to create a unit test? |
|
@tjruwase Yea sure ill add one into the PR do you have any preference to location? maybe unit/runtime/utils ? |
|
How about tests/unit/checkpoint/test_zero_optimizer.py? |
|
@microsoft-github-policy-service agree @tjruwase Added the unit test, built it as a ZeRO Non Distributed since its not needed for the test. Maybe allows some expansion for other things to be applied in there. Also split the original patch into its own method to allow the unit test, since non permissible file/folders only exist in certain circumstances (Azure Storage File Share, etc.), none of which are in Github Actions. 🍻 |
deepspeed/runtime/engine.py
Outdated
| # make executable (safeguard for file shares - Azure as example) | ||
| try: | ||
| os.chmod(dst, os.stat(dst).st_mode | stat.S_IEXEC) | ||
| except Exception as e: |
There was a problem hiding this comment.
Is it worth making this specific to except PermissionsError as e?
There was a problem hiding this comment.
We can and initially I did.. then I thought maybe there's some other circumstance that could arise so it's a catch all.
Up to you guys, I'm happy to make the change.
- looks like the formatter needs me to fix a typo and doesn't want the trailing white space on the file so an update is needed either way
There was a problem hiding this comment.
@M-Chris, this should help with formatting: https://github.com/microsoft/DeepSpeed/blob/master/CONTRIBUTING.md#contributing
There was a problem hiding this comment.
I prefer more specific exceptions. Thanks!
| This is presented for use-cases like Azure Storage File Share (where permissions are not allowed) | ||
| We use a fake file for this test (file not existing would present a similar issue as not being able to chmod) | ||
| """ | ||
| fake_recovery_script_dst = os.path.join("tmp", "zero_to_fp32.py") |
There was a problem hiding this comment.
I guess for the test we'd need to add FileNotFoundError in order for this test to pass?
There was a problem hiding this comment.
yup caught it and pushed already - you were faster than my comment 😄
|
@loadams Added (FileNotFoundError, PermissionError) I should have thought about that.. since the unit test requires a not found, since there's no way to validate file shares in Github Actions to my knowledge. |
I agree, I'm not aware of any way to do that, so catching both makes sense. |
This Pull Request introduces a try-except block around the
os.chmodcall used to change file permissions in the DeepSpeed engine. The change ensures that aPermissionError(or any other unexpected exception) does not halt the program's execution but instead logs a warning message.This is particularly relevant when working with read-only filesystems or shared storage where permission modification might not be allowed.
The new exception handling allows for greater robustness in various environments without compromising the overall functionality of the system. A detailed warning message has been added to inform the user of any issues encountered during the chmod operation, aiding in debugging and future refinement of the code.