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

Misleading Exceptions #58

Closed
andersbogsnes opened this issue May 25, 2020 · 5 comments
Closed

Misleading Exceptions #58

andersbogsnes opened this issue May 25, 2020 · 5 comments
Labels

Comments

@andersbogsnes
Copy link
Contributor

I just updated to 0.3.0 and am adapting my own libraries to these changes - great to have support for azure blob storage 12!

I noticed that some of the exceptions are overly broad and gives the end-user a misleading error message.

For example, passing invalid credentials will raise a FileNotFoundError, suppressing Azure Blob Storages AuthorizationFailure.

Example:
While running Azurite:

>>> connection_string = "DefaultEndpointsProtocol=http;AccountName=devstoreaccount1;AccountKey=test;BlobEndpoint=http://127.0.0.1:10000/devstoreaccount1;" # Notice invalid AccountKey here

>>> abfs = AzureBlobFileSystem(account_name="test", connection_string=connection_string)
>>> abfs.ls(".")
azure.core.exceptions.HttpResponseError: Server failed to authenticate the request. Make sure the value of the Authorization header is formed correctly including the signature.
RequestId:63c0f3c2-d764-48dd-b7fa-522322a61e79
Time:2020-05-25T14:01:15.322Z
ErrorCode:AuthorizationFailure
Error:
  
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "<input>", line 1, in <module>
  File "/home/anders/.pyenv/versions/feature_env/lib/python3.8/site-packages/adlfs/core.py", line 534, in ls
    raise FileNotFoundError(f"File {path} does not exist!!")
FileNotFoundError: File  does not exist!!

I would expect to either have a custom exception I can catch, or to let through the Azure exception so that the user can handle it.

As-is, I can't handle an Authentication issue separately from other issues when using adlfs in my libraries.

@andersbogsnes
Copy link
Contributor Author

andersbogsnes commented Jun 8, 2020

I'd be happy to work on a PR if there is an agreement that this is an issue 😄

@hayesgb
Copy link
Collaborator

hayesgb commented Jun 13, 2020

Thanks @andersbogsnes . Agree this is an issue, and if you have some time to work on it that would be great.

However, I added one unit test (test_ls_0), which calls fs.ls("."), and if I
update the ls() method to simply allow the Exception to pass through, but this then causes some of the glob tests to fail. For the moment, I will leave the FileNotFoundError in place, but I've propagated this up to master with the test marked as xfail.

@hayesgb hayesgb added the bug label Jun 13, 2020
@andersbogsnes
Copy link
Contributor Author

Ran into some more issues with exceptions here: https://github.com/dask/adlfs/blob/07aabb7beea34b640c75991f07142154f8f39b3c/adlfs/core.py#L754

(It's trying to raise a string, which errors)

I'm working on a PR in my downtime, but was wondering if there's a need to introduce an exception hierarchy of some sort. Does fsspec have an opinion on that? What type of exception should be raised here?

@hayesgb
Copy link
Collaborator

hayesgb commented Jul 8, 2020

The only error I have seen in this block is a ResourceNotFound error. If we get back something else, we should probably raise a RuntimeError in this block, and propagate the error message {e} back to the user.

@hayesgb
Copy link
Collaborator

hayesgb commented Jul 10, 2020

Closed on PR#72

@hayesgb hayesgb closed this as completed Jul 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants