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 84.61538% with 4 lines in your changes missing coverage. Please review.

Project coverage is 82.31%. Comparing base (f765128) to head (abee48e).
Report is 3 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #641      +/-   ##
==========================================
+ Coverage   82.30%   82.31%   +0.01%     
==========================================
  Files          72       72              
  Lines        7430     7446      +16     
==========================================
+ Hits         6115     6129      +14     
- Misses       1315     1317       +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!