More information in error message when _get_endpoint requests fail

[stvoutsin]

Summary

Currently when a user provides an invalid TAP URL or encounters connection issues during instantiation:
tap_service = pyvo.dal.TAPService(baseurl) the error message is:

DALServiceError: No working capabilities endpoint provided

We've found that this is slightly confusing for our users and lacks a bit of information on what went wrong.

Changes

This PR:

• Provides more context to the error messages
• Shows what URLs were attempted when trying to connect to the TAP service
• Suggests possible fixes (e.g., verify URL is correct, check service is running)
• Adds test coverage for various connection failure scenarios like:
  • Invalid/malformed URLs
  • HTTP error responses (403, 500, 502, 503)
  • Network-level connection failures

Other Changes

• In the process I've also fixed a couple of issues with the docstrings where mostly there were missing or incorrect params.
• Added some classes to __all__ in dal.query

Example

Example improved error message:

pyvo.dal.exceptions.DALServiceError: Cannot find TAP service at 'https://data-dev.lsst.cloud/api/setap'.

Unable to access the capabilities endpoint at:
- https://data-dev.lsst.cloud/api/setap/capabilities: 404 Not Found
- https://data-dev.lsst.cloud/api/capabilities: 404 Not Found

This could mean:
1. The service URL is incorrect
2. The service is temporarily unavailable
3. The service doesn't support this protocol

Related Issues

#125
#586

Interested to hear if these changes make sense or if you have any objections / alternative approaches you'd rather go with.

[codecov]

Codecov Report

Attention: Patch coverage is 90.19608% with 5 lines in your changes missing coverage. Please review.

Project coverage is 82.47%. Comparing base (5a9073d) to head (6b67d19).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pyvo/dal/vosi.py	93.18%	3 Missing ⚠️
pyvo/dal/tap.py	66.66%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #641      +/-   ##
==========================================
+ Coverage   82.40%   82.47%   +0.07%     
==========================================
  Files          72       72              
  Lines        7438     7480      +42     
==========================================
+ Hits         6129     6169      +40     
- Misses       1309     1311       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[bsipocz]

Thanks for the PR. I have the feeling that we may have one or a couple of issues open about this, if you're up for finding them please link them in the OP comment.

[stvoutsin]

Thanks for the PR. I have the feeling that we may have one or a couple of issues open about this, if you're up for finding them please link them in the OP comment.

The only related issues I found were:

• BUG: return a less cryptic error for unavailable TAP url  #586
• Improved error handling #125

Although the issue description in both is a bit different then the specific change this PR is attempting to address.

[bsipocz]

These all look to be good improvements, thanks!

I hold off merging as I expect @andamian or @msdemlei may want to have a quick look, too.

[bsipocz]

Thanks! Test failures are unrelated upstream issues that are expected to go away once upstream deployment is done.

[msdemlei]

On Fri, Jan 24, 2025 at 10:33:20AM -0800, stvoutsin wrote: @stvoutsin commented on this pull request. > @@ -32,17 +32,43 @@ def _get_endpoint_candidates(self, endpoint): return [f'{curated_baseurl}/{endpoint}', url_sibling(curated_baseurl, endpoint)] def _get_endpoint(self, endpoint): - for ep_url in self._get_endpoint_candidates(endpoint): + """Attempt to connect to service endpoint""" + attempted_urls = [] + + try: + candidates = self._get_endpoint_candidates(endpoint) + except Exception as e: I've added the exceptions I'd expect to be thrown here (`ValueError`, `TypeError` & `AttributeError`). Let me know if you think I've missed any

AttributeError would catch the case when someone passes a non-String. Personally, I probably would not bother catching and explaining these, but since these days the original traceback is preserved, that's not dramatic, and so that's fine for me, and I can see when the explanation given is actually helpful. I've tried to elicit a ValueError or a TypeError from urlparse which, I think, is the main source of exceptions here but failed. Hence, based on my philosophy that it's usually wise to let "unusual" exceptions propagate to the user I'd rather not catch them. The logic is somewhat like "If you don't know why you would see an exception here, don't try to explain it; a bad error message is better than a wrong one.", which perhaps is a corollary to then Zen of Python's "In the face of ambiguity, refuse the temptation to guess." Other than that, I think all of this is an improvement, so let's go. And let's, at the IVOA level, try to get rid of the horrible _get_endpoint_candidates in the first place. I'm still trying to get traction for <http://ivoa.net/documents/caproles/>...

[stvoutsin]

AttributeError would catch the case when someone passes a non-String. Personally, I probably would not bother catching and explaining these, but since these days the original traceback is preserved, that's not dramatic, and so that's fine for me, and I can see when the explanation given is actually helpful. I've tried to elicit a ValueError or a TypeError from urlparse which, I think, is the main source of exceptions here but failed. Hence, based on my philosophy that it's usually wise to let "unusual" exceptions propagate to the user I'd rather not catch them. The logic is somewhat like "If you don't know why you would see an exception here, don't try to explain it; a bad error message is better than a wrong one.", which perhaps is a corollary to then Zen of Python's "In the face of ambiguity, refuse the temptation to guess." Other than that, I think all of this is an improvement, so let's go. And let's, at the IVOA level, try to get rid of the horrible _get_endpoint_candidates in the first place. I'm still trying to get traction for http://ivoa.net/documents/caproles/...

Thanks for the comments!
I generally see your point and agree with the principle of "don't catch exceptions you don't understand".
The only case I was considering ValueError is an invalid IPv6 address:

tap_service = pyvo.dal.TAPService(baseurl="http://[invalid-url", session=auth)

If we just let this, as well as the AttributeError caused by an invalid type get propagated by removing that catch, we end up exposing the internal implementation details and produce deep tracebacks that a user will have to decode.

In the most recent commit, I've slightly modified by removing the TypeError catch, and more properly propagate the error that was triggered.
Just to give a better idea of the user's pov:

Approach 1 (Propagate Errors)

AttributeError:

tap_service = pyvo.dal.TAPService(baseurl=1, session=auth)

tap_service = pyvo.dal.TAPService(baseurl=1,
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "pyvo/dal/tap.py", line 129, in __init__
    self._session.update_from_capabilities(self.capabilities)
                                           ^^^^^^^^^^^^^^^^^
  File "lib/python3.12/site-packages/astropy/utils/decorators.py", line 850, in __get__
    val = self.fget(obj)
          ^^^^^^^^^^^^^^
  File "pyvo/dal/vosi.py", line 116, in capabilities
    return vosi.parse_capabilities(self._capabilities().read)
                                   ^^^^^^^^^^^^^^^^^^^^
  File "pyvo/utils/decorators.py", line 9, in wrapper
    raw = func(*args, **kwargs)
          ^^^^^^^^^^^^^^^^^^^^^
  File "pyvo/dal/vosi.py", line 112, in _capabilities
    return self._get_endpoint('capabilities')
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "pyvo/dal/vosi.py", line 37, in _get_endpoint
    candidates = self._get_endpoint_candidates(endpoint)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "pyvo/dal/vosi.py", line 22, in _get_endpoint_candidates
    urlcomp = urlparse(self.baseurl)
              ^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/urllib/parse.py", line 394, in urlparse
    url, scheme, _coerce_result = _coerce_args(url, scheme)
                                  ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/urllib/parse.py", line 133, in _coerce_args
    return _decode_args(args) + (_encode_result,)
           ^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/urllib/parse.py", line 117, in _decode_args
    return tuple(x.decode(encoding, errors) if x else '' for x in args)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/urllib/parse.py", line 117, in <genexpr>
    return tuple(x.decode(encoding, errors) if x else '' for x in args)
                 ^^^^^^^^
AttributeError: 'int' object has no attribute 'decode'

Value Error

tap_service = pyvo.dal.TAPService(baseurl="http://[invalid-url", session=auth)

tap_service = pyvo.dal.TAPService(baseurl="http://[invalid-url",
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "pyvo/dal/tap.py", line 129, in __init__
    self._session.update_from_capabilities(self.capabilities)
                                           ^^^^^^^^^^^^^^^^^
  File "lib/python3.12/site-packages/astropy/utils/decorators.py", line 850, in __get__
    val = self.fget(obj)
          ^^^^^^^^^^^^^^
  File "pyvo/dal/vosi.py", line 116, in capabilities
    return vosi.parse_capabilities(self._capabilities().read)
                                   ^^^^^^^^^^^^^^^^^^^^
  File "pyvo/utils/decorators.py", line 9, in wrapper
    raw = func(*args, **kwargs)
          ^^^^^^^^^^^^^^^^^^^^^
  File "pyvo/dal/vosi.py", line 112, in _capabilities
    return self._get_endpoint('capabilities')
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "pyvo/dal/vosi.py", line 37, in _get_endpoint
    candidates = self._get_endpoint_candidates(endpoint)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "pyvo/dal/vosi.py", line 22, in _get_endpoint_candidates
    urlcomp = urlparse(self.baseurl)
              ^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/urllib/parse.py", line 395, in urlparse
    splitresult = urlsplit(url, scheme, allow_fragments)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/urllib/parse.py", line 497, in urlsplit
    raise ValueError("Invalid IPv6 URL")
ValueError: Invalid IPv6 URL

Approach 2 (Explicit handling of Errors):

  try:
      candidates = self._get_endpoint_candidates(endpoint)
  except (ValueError, AttributeError) as e:
      raise DALServiceError(
          f"Cannot construct endpoint URL from base '{self.baseurl}' and endpoint '{endpoint}'. "
          f"{type(e).__name__}: {str(e)}"
      ) from e

AttributeError:

tap_service = pyvo.dal.TAPService(baseurl=1, session=auth)

    tap_service = pyvo.dal.TAPService(baseurl=1,
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "pyvo/dal/tap.py", line 131, in __init__
    raise DALServiceError(f"Cannot find TAP service at '"
pyvo.dal.exceptions.DALServiceError: Cannot find TAP service at '1'.	

Cannot construct endpoint URL from base '1' and endpoint 'capabilities'. AttributeError: 'int' object has no attribute 'decode'

Value Error

tap_service = pyvo.dal.TAPService(baseurl="http://[invalid-url", session=auth)

    tap_service = pyvo.dal.TAPService(baseurl=tap_url,
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "pyvo/dal/tap.py", line 131, in __init__
    raise DALServiceError(f"Cannot find TAP service at '"
pyvo.dal.exceptions.DALServiceError: Cannot find TAP service at 'http://[invalid-url'.

Cannot construct endpoint URL from base 'http://[invalid-url' and endpoint 'capabilities'. ValueError: Invalid IPv6 URL

Interested to hear your thoughts on this, I think either case is fine really because both are really obscure and rare cases that users should rarely encounter.
I was more interested in making sure the connection errors are clearly indicated to the user, for this I'm happy to go with whichever approach you prefer.

I'll read up on the caproles doc, but I agree the need for that _get_endpoint_candidates step is awkward so definitely something that should indeed be addressed at the IVOA level.

[msdemlei]

On Mon, Jan 27, 2025 at 04:08:24PM -0800, stvoutsin wrote: Interested to hear your thoughts on this, I think either case is fine really because both are really obscure and rare cases that users should rarely encounter.

I had missed the ValueError for IPv6, and I agree it's better UX if we catch these two. Thanks!

[andamian]

This is a great improvement. I don't know how far we want to go but for the uninitiated users we could provide more details:

• ConnectionError is raised when the service cannot be reached: wrong URL, DNS or other networking problem, server down (?)
• HTTP 5** errors are generally server errors that the user cannot do much about. The response bodies could provide more details. I think somewhere else in the code we attached the response body to the exception if it's in text format but I forgot where exactly. But users could be directed to check the message body with a browser. Also 503 might have a Retry-After header that can useful info. The code might be returned when service is in maintenance.
• HTTP 4** in general have something to do with the request so user might be able to act on although in this case is such a simple request that most of error codes can be considered server errors.

These are just suggestions

[bsipocz]

Something has went wrong with the rebase/squashing and now this contains unrelated changes.

[stvoutsin]

Something has went wrong with the rebase/squashing and now this contains unrelated changes.

My bad, I forgot I was working off a fork so I messed up the rebase. Should be good now, however I think I may have to add some more test cases since there are some additional changes based on suggestions from @andamian

[stvoutsin]

This is a great improvement. I don't know how far we want to go but for the uninitiated users we could provide more details:

• ConnectionError is raised when the service cannot be reached: wrong URL, DNS or other networking problem, server down (?)
• HTTP 5** errors are generally server errors that the user cannot do much about. The response bodies could provide more details. I think somewhere else in the code we attached the response body to the exception if it's in text format but I forgot where exactly. But users could be directed to check the message body with a browser. Also 503 might have a Retry-After header that can useful info. The code might be returned when service is in maintenance.
• HTTP 4** in general have something to do with the request so user might be able to act on although in this case is such a simple request that most of error codes can be considered server errors.

These are just suggestions

Thanks for the feedback! I've taken your suggestions and modified the error handling a bit, to provide more context based on the various errors we may encounter, i.e. check for retry-after for 503s and provide a more meaningful message for ConnectionError.

Let me know if the changes look reasonable or if I've missed anything and I'll be happy to adjust it.

[andamian]

This is more user friendly especially with less experience users. Thank you for your contribution @stvoutsin.