make timeout work consistently across services and not misuse the configuration system

[eteq]

Nearly all of the astroquery services use a timeout argument, typically as part of send_request or similar. However, it seems like a few different ways of specifying this are available. I note gama has a timeout argument on the class, and lcogt has the same but it's TIMEOUT. Nrao meanwhile, looks similar, but is subtly different because it uses the module-level instance Nrao.timeout instead of self.timeout. Others (e.g. sdss) instead have a default but allow the user to specify the timeout as an argument to a query function.

Moreover, in the cases I've looked at that use the configuration system, there's what appears to be a mis-use of the configuration system happening. (as an example, checkout out sdss/core.py). The TIMEOUT attibute is specified at the class level, so the configuration item only gets touched at import time. If a user changes the configuration item as the astropy docs recommends, the code won't notice, because conf.timeout is never again accessed. Worse yet, the TIMEOUT attribute is only used to specify the default, so changing that also does nothing.

This is very confusing from a user perspective - these should all be done uniformly. Personally I advocate for the "use a default from conf.timeout, unless specified by the user". So the queries woult all have timeout=None as an argument and usage would look like:

if timeout is None:
    timeout = conf.timeout

commons.send_request(...,timeout=timeout,...)

@keflavich - do you have a sense if some of this is intentional? I (or someone else) could pretty easily go through and uniformly implement what I'm describing above, but maybe I missed some discussion of a reason for why all these different techniques are in use?

[keflavich]

@eteq - No, none of this inconsistency is intentional. I think we've gone through different phases where on any given module a different solution was, perhaps unconsciously, agreed upon.

• A configuration item for each module
• A class-level timeout that uses the configuration item
• A query function timeout argument

I think all 3 can be useful, with priority going from bottom to top of that list. So instead of the snippet you used, it would be more like:

class Foo:
    timeout = conf.timeout
    def query(timeout=None):
        if timeout is None:
            timeout = self.timeout

The class-level timeout makes some (limited) sense because class-level URLs are configurable, and you might want to have a different but broad-scope timeout for each mirror. The per-query timeout really seems necessary to me.

This would only require the timeout = self.timeout snippet to be implemented in query.py as long as all modules inherit from BaseQuery (at the moment, not all do), so it's not too messy.

[keflavich]

This was yet another instance (the first?) of problems with the configuration system (e.g., #3018, #2993).