MAINT: cleanup configitem defaultvalues

[bsipocz]

The discussion in #3413 made me wonder if we need to review the configs for each and every module and see if the ConfigItem defaultvalues are used correctly, namely if it's a list it won't be possible to override at run-time.

And that behaviour is IMO disadvantageous in each case when we don't deal with mirrors and multiple servers, for example in the case of IRSA we routinely test server side changes by runtime changing the server url to the test server, and I see no reason to ship that url as a possibility in the list.

cc @keflavich for opinion and as he may remember why we kept using lists for non-mirrored services.

[keflavich]

I have no immediate recollection.

The reason to have multiple values is to list mirrors, but frankly that's more of a documentation role.

Why does using a list prevent runtime override? Does config treat that as the list of all allowed options? If that's the case, I'd be in favor of changing (and documenting) the behavior. It would be valuable to make possible something like this:

from astroquery.simbad import Simbad, conf
for mirror in conf.mirrors:
    sim = Simbad(server_url=mirror)
    <do query>

Right now, you can't do that, which is not good

from astroquery.simbad import SimbadClass, conf
for mirror in conf.servers_list:
    conf.server = mirror
    sim = Simbad()
    print(sim.SIMBAD_URL)

instead you have to do

from astroquery.simbad import SimbadClass, conf
for mirror in conf.servers_list:
    sim = Simbad()
    sim.SIMBAD_URL = 'https://' + mirror + '/simbad/sim-script'

The server should definitely be more nicely configurable at runtime. So I think a sweeping change is warranted here - but let's spend a little time talking and thinking about design.

[bsipocz]

OK, thanks for the brainstorm, and I agree that I should not rush this though the next release. -- but preferably we need to fix this sooner rather than later.

Yes, that behaviour is OK for the case when indeed there are legit mirros, but it's insufficient for cases when there are alternate servers we don't want to expose in the configs or when there are url changes and we want to promote workarounds for users between releases.

Or would you say it's preferably to change it on the instance level?

Right now:

>>> from astroquery.image_cutouts.first import FirstClass, conf
>>> conf.server = conf.server = 'https://sundog.stsci.edu/cgi-bin/firstcutout'
...
TypeError: Provided value for configuration item server not valid: the value "https://sundog.stsci.edu/cgi-bin/firstcutout" is unacceptable.

This kinds of work, but one need to remember to change it on the instance every time (and also the code needs to be super consistent that it's not picking up the default config value again -- not sure if all modules would pass this

>>> from astroquery.image_cutouts.first import FirstClass, conf
>>> fc = FirstClass()
>>> fc.URL = 'https://sundog.stsci.edu/cgi-bin/firstcutout'
>>> from astropy.coordinates import SkyCoord
>>> import astropy.units as u
>>> fc.get_images_async(SkyCoord(162.530, 30.677, unit=(u.deg, u.deg), frame='icrs'), image_size='1 arcmin')
<Response [200]>

But this is how it works for IRSA and it's a rather convenient without the need to do changes in the middle of the code (e.g. super easy to test things or tell people how to change things to test it themselves (notebooks, code, etc))

>>> from astroquery.ipac.irsa import IrsaClass, conf
>>> conf.sia_url = 'https://irsadev.ipac.caltech.edu/SIA'
>>> Irsa = IrsaClass()
# Rest of the code is exactly the same as it would be with the default URL, and URLs are propagated properly
>>> Irsa.sia_url
'https://irsadev.ipac.caltech.edu/SIA'

[keflavich]

Yeah, I'd like there to be multiple ways to achieve this:

• At the instance level. This is convenient if, say, a mirror is down and you want to switch to another one. It's also the right way (imo) to do a try: mirror1 except: mirror2 setup
• At the config level, for the case you've shown

It would be nice if the interface looked lie:

inst_with_new_mirror = Class(url=mirror)

for the former case (it does not right now!)

and for the second case, it should look like the example you gave, i.e. (copy pasted exactly):

>>> from astroquery.ipac.irsa import IrsaClass, conf
>>> conf.sia_url = 'https://irsadev.ipac.caltech.edu/SIA'
>>> Irsa = IrsaClass()
# Rest of the code is exactly the same as it would be with the default URL, and URLs are propagated properly
>>> Irsa.sia_url
'https://irsadev.ipac.caltech.edu/SIA'

[ManonMarchand]

from astroquery.simbad import Simbad, conf
for mirror in conf.mirrors:
sim = Simbad(server_url=mirror)

Right now, you can't do that, which is not good

from astroquery.simbad import SimbadClass, conf
for mirror in conf.servers_list:
conf.server = mirror
sim = Simbad()
print(sim.SIMBAD_URL)

instead you have to do

from astroquery.simbad import SimbadClass, conf
for mirror in conf.servers_list:
sim = Simbad()
sim.SIMBAD_URL = 'https://' + mirror + '/simbad/sim-script'

This behavior changed last year. Now you have to do:

from astroquery.simbad import Simbad
simbad = Simbad
simbad.server = "foo"

ValueError: 'foo' does not correspond to a SIMBAD server, the two existing ones are ['simbad.cds.unistra.fr', 'simbad.harvard.edu'].

Which is read from conf.servers_list.

We could add server as an init parameter to reach the behavior you wanted?

[bsipocz]

I would say this is the correct behaviour for simbad, you have a list of valid server names and only allow the users to choose from those.
For IRSA I want the more free-form one as we don't have mirror sites, but I do want the easy possibility to test with the test/dev servers.

And the issue that started this all was again about a service that doesn't have mirrors and thus not using the mirror list machinery would have helped us to give a way to users to easily override the URL run time without changing the config themselves.

So I would say, if you don't expect a usecase for a simbad url that is not listed in the mirror list, then it's all good as is.

[ManonMarchand]

Not, it's all good, we edit the __init__.py file on our side when we play with SIMBAD's test server

[bsipocz]

I do have other non-devs doing testings, so for them it's much easier to not tinker with the astroquery source code, nor to edit the config file in their home directory, but to have a one liner override in their notebooks. IMO both solution is fine and should be left at the discretion of the archives, but with defaulting of the non-list/non-mirror.

Overall, I think the template can use a bit of a work, or more likely being removed altogether in its current form 😅

[bsipocz]

Btw, welcome back 🎉