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

Raise exceptions more #351

Merged
merged 1 commit into from
Nov 22, 2023
Merged

Conversation

jrbourbeau
Copy link
Collaborator

There are a few places where we're currently printing a message and returning None, [], etc. when something goes wrong. It's nice that there's typically an informative message, but it relatively easy to ignore these messages (guilty myself), and can lead to unexpected downstream errors (e.g. downstream code is expecting a list of strings, but you give it None instead).

Here's an example

In [5]: result = earthaccess.open(["notavalidprotocol://myfile.nc"])
Schema for notavalidprotocol://myfile.nc is not recognized, must be an HTTP or S3 URL

In [6]: type(result)
Out[6]: NoneType

This PR proposes we raise exceptions in these cases (still with an informative message) to properly signal "Hey, something went wrong here". This should make it more clear where things are going awry.

With the changes in this PR, the above example looks like this instead

In [2]: result = earthaccess.open(["notavalidprotocol://myfile.nc"])
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Cell In[2], line 1
----> 1 result = earthaccess.open(["notavalidprotocol://myfile.nc"])

File ~/projects/nsidc/earthaccess/earthaccess/api.py:197, in open(granules, provider)
    184 def open(
    185     granules: Union[List[str], List[earthaccess.results.DataGranule]],
    186     provider: Optional[str] = None,
    187 ) -> List[AbstractFileSystem]:
    188     """Returns a list of fsspec file-like objects that can be used to access files
    189     hosted on S3 or HTTPS by third party libraries like xarray.
    190
   (...)
    195         a list of s3fs "file pointers" to s3 files.
    196     """
--> 197     results = earthaccess.__store__.open(granules=granules, provider=provider)
    198     return results

File ~/projects/nsidc/earthaccess/earthaccess/store.py:281, in Store.open(self, granules, provider)
    272 """Returns a list of fsspec file-like objects that can be used to access files
    273 hosted on S3 or HTTPS by third party libraries like xarray.
    274
   (...)
    278     a list of s3fs "file pointers" to s3 files.
    279 """
    280 if len(granules):
--> 281     return self._open(granules, provider)
    282 return []

File ~/mambaforge/envs/earthaccess-dev/lib/python3.9/site-packages/multimethod/__init__.py:315, in multimethod.__call__(self, *args, **kwargs)
    313 func = self[tuple(func(arg) for func, arg in zip(self.type_checkers, args))]
    314 try:
--> 315     return func(*args, **kwargs)
    316 except TypeError as ex:
    317     raise DispatchError(f"Function {func.__code__}") from ex

File ~/projects/nsidc/earthaccess/earthaccess/store.py:383, in Store._open_urls(self, granules, provider, threads)
    381     data_links = granules
    382 else:
--> 383     raise ValueError(
    384         f"Schema for {granules[0]} is not recognized, must be an HTTP or S3 URL"
    385     )
    386 if self.auth is None:
    387     raise ValueError(
    388         "A valid Earthdata login instance is required to retrieve S3 credentials"
    389     )

ValueError: Schema for notavalidprotocol://myfile.nc is not recognized, must be an HTTP or S3 URL

Which seems more like what I'd expect.

Copy link

Binder 👈 Launch a binder notebook on this branch for commit 95be04c

I will automatically update this comment whenever this PR is modified

@MattF-NSIDC
Copy link

💯 💯 💯

I'm going to have to look closer on non-work time. Thank you for digging in to this. This is much needed.

@MattF-NSIDC
Copy link

I wanted to look over the weekend, but still struggling to find the time. We could probably use another maintainer 😆

Copy link
Member

@betolink betolink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good to me!

@betolink betolink merged commit f01b311 into nsidc:main Nov 22, 2023
8 checks passed
@jrbourbeau jrbourbeau deleted the raise-more-in-store branch November 27, 2023 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants