astroquery icon indicating copy to clipboard operation
astroquery copied to clipboard

Issue changing configuration at runtime

Open astrofrog opened this issue 1 year ago • 1 comments

The following should work to change e.g. the astrometry.net server:

from astroquery.astrometry_net import AstrometryNet, conf
conf.server = 'http://localhost:8080
ast = AstrometryNet(...)

but does not, because configuration settings are read at class definition time:

class AstrometryNetClass(BaseQuery):
    """
    Perform queries to the astrometry.net service to fit WCS to images
    or source lists.
    """
    URL = conf.server
    TIMEOUT = conf.timeout
    API_URL = url_helpers.join(URL, 'api')

I noticed this on astrometry.net because this is the one that I wanted it to work for, but I noticed this pattern in other classes too. We should instead be reading the configuration at runtime at the last minute when these values are needed.

However, before I open a PR to try and fix this, I wanted to check if there is a deliberate reason why the above was done?

astrofrog avatar Apr 22 '24 12:04 astrofrog

The pattern I've promoted is instead

from astroquery.astrometry_net import AstrometryNet, conf
ast = AstrometryNet(...)
ast.URL = 'http://localhost:8080'

so I designed modules to support that approach, expecting that configuration changes would be done via configuration files, not modified at runtime, while individual modules would have their URLs updated at runtime. This latter approach is useful if you want to use different URLs with different instances of the module, e.g.:

from astroquery.astrometry_net import AstrometryNet, conf
ast1 = AstrometryNet(...)
ast1.URL = host1
ast2 = AstrometryNet(...)
ast2.URL = host2

This latter use case would be more awkward using the conf-only approach.

However, I would be in favor of supporting both approaches.

keflavich avatar Apr 22 '24 12:04 keflavich

Closing this as the current behavior is intended, but there is room to change the design if there is good reason to do so. Let's open a new issue if that's the case.

keflavich avatar Sep 07 '24 18:09 keflavich