-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
[Datasets] Handle any error code when encountering AWS S3 credential error #26669
Conversation
@matthewdeng - could you help take a look when you have time? Thanks. |
# Specially handle AWS error when reading files, to give a clearer error | ||
# message to avoid confusing users. The real issue is most likely that the AWS | ||
# S3 file credentials have not been properly configured yet. |
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 is just move the original comment literally without any change. Feel make more sense to comment inside the if
branch, as we plan to extend the function to apply to other file system such as GCS in the future.
# Specially handle AWS error when reading files, to give a clearer error message | ||
# to avoid confusing users. The real issue is most likely that the AWS S3 file | ||
# credentials have not been properly configured yet. | ||
def _handle_read_os_error(error: OSError, paths: Union[str, List[str]]) -> str: |
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.
Add a quick note to make it explicit that this is not comprehensive yet, and should be extended as more errors arise?
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.
@matthewdeng - sure, added.
@pcmoritz, @dmatrix - this should fix #26672, please let me know if the error message below is intuitive enough, thanks.
|
Thanks @scv119 and @matthewdeng for review! |
Do you know if there are any resources describing what these error codes (e.g. 15, 100) mean? I feel like it's still not clear which issue is occurring. |
@matthewdeng - my guess when user encounters the issue, is they will (1). first check the file exists ( From @pcmoritz 's link, it looks like error code 15 is |
@pcmoritz and I briefly chatted about how we could improve the UX so it is easier for the user to check if the file exists and their credentials. One idea that comes from Philipp's manual debugging is to show them the s3 CLI/SDK command they can run to get a more verbose error message (e.g. #19799 (comment) or #19799 (comment)). |
…error (ray-project#26669) As a followup of ray-project#26619 (comment) and ray-project#26619 (comment), here we change from PermissionError to OSError, to be consistent as original error, and also change function name from _handle_read_s3_files_error to _handle_read_os_error, which is more general that we can handle other file systems such as GCS in the future. Also change to hanlde any error message with pattern AWS Error [code xxx]: No response body as new issue with error code 100 is raised in ray-project#26672 . Signed-off-by: Xiaowei Jiang <[email protected]>
As followup of #26669 (comment), we want to add AWS CLI command information into S# credential error message, so users have a better idea to further debug the read issue.
…oject#26789) As followup of ray-project#26669 (comment), we want to add AWS CLI command information into S# credential error message, so users have a better idea to further debug the read issue. Signed-off-by: Rohan138 <[email protected]>
…error (ray-project#26669) As a followup of ray-project#26619 (comment) and ray-project#26619 (comment), here we change from PermissionError to OSError, to be consistent as original error, and also change function name from _handle_read_s3_files_error to _handle_read_os_error, which is more general that we can handle other file systems such as GCS in the future. Also change to hanlde any error message with pattern AWS Error [code xxx]: No response body as new issue with error code 100 is raised in ray-project#26672 . Signed-off-by: Stefan van der Kleij <[email protected]>
…oject#26789) As followup of ray-project#26669 (comment), we want to add AWS CLI command information into S# credential error message, so users have a better idea to further debug the read issue. Signed-off-by: Stefan van der Kleij <[email protected]>
Why are these changes needed?
As a followup of #26619 (comment) and #26619 (comment), here we change from
PermissionError
toOSError
, to be consistent as original error, and also change function name from_handle_read_s3_files_error
to_handle_read_os_error
, which is more general that we can handle other file systems such as GCS in the future.Also change to hanlde any error message with pattern
AWS Error [code xxx]: No response body
as new issue with error code 100 is raised in #26672 .Tested the example in #26672 and verified it shows new error message now:
Related issue number
Closes #26672 .
Checks
scripts/format.sh
to lint the changes in this PR.