Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changing config items at runtime is broken #2291

Open
bsipocz opened this issue Feb 10, 2022 · 8 comments
Open

Changing config items at runtime is broken #2291

bsipocz opened this issue Feb 10, 2022 · 8 comments

Comments

@bsipocz
Copy link
Member

bsipocz commented Feb 10, 2022

Changing config values at runtime should be possible, yet it's not working: https://docs.astropy.org/en/stable/config/index.html#changing-values-at-runtime

>>> from astroquery.simbad import conf
>>> conf.server='simbad.harvard.edu'
>>> from astroquery.simbad import Simbad
>>> Simbad.SIMBAD_URL
'http://simbad.u-strasbg.fr/simbad/sim-script'
@bsipocz
Copy link
Member Author

bsipocz commented Feb 10, 2022

cc @saimn and @pllim if you could spot something inherently wrong in our usage, as I recall you have been working on the config system most recently.

@bsipocz bsipocz changed the title Changing config items runtime is broken Changing config items at runtime is broken Feb 10, 2022
@eerovaher
Copy link
Member

@eteq has already provided an explanation of the problem together with an outline of a solution in #544. See also https://docs.astropy.org/en/stable/config/index.html#usage-tips

@pllim
Copy link
Member

pllim commented Feb 10, 2022

Indeed. As you can see here:

SIMBAD_URL = 'http://' + conf.server + '/simbad/sim-script'

SIMBAD_URL is a class variable, which means it grabs the config value pretty much at import time (I think) and it does not update when the config changes. If you really want people to be able to change SIMBAD_URL at runtime, it should not be a class variable but a config item itself.

p.s. Or maybe a @property that re-evaluates at runtime.

@bsipocz
Copy link
Member Author

bsipocz commented Feb 10, 2022

Hmm. I hoped that delaying the import of the class does the trick, but you're absolutely right that it shouldn't.

This brings us back to the same discussion we had recently of what should be on the class and what on the instance level. Maybe even the URLs should be instance, at least when there is an option to choose between the mirrors?

Do you see any downside of having them as a property instead?

OTOH, I would be down for the idea of not allowing changing the mirror at runtime but only doing that at the config level.

@pllim
Copy link
Member

pllim commented Feb 10, 2022

Since Python does not have true "private" attributes, personally, I think property vs config is just a matter of preference.

If this Simbad server is used throughout astroquery, I think it makes more sense as config. But if it is just used inside this one class, I am not sure if it matters much. But then again, I am unfamiliar with all the possible use cases of astroquery objects.

@bsipocz
Copy link
Member Author

bsipocz commented Feb 10, 2022

Again, a very good point. Not much atm, but indeed the simbad module is cross used in other modules so for that use case the config system has to be used and there is no runtime way to change the url.

@eerovaher
Copy link
Member

The astropy config system includes features like a built-in context manager for temporarily changing config item values (very useful for testing) and input validation. For these reasons I would prefer to use the astropy config system as much as possible. Not all users can be expected to be familiar with it, so there should also be some other way of specifying values at runtime, whether it be through function arguments or class or instance attributes. But using the config system at runtime should work in all the subpackages. #2292 provides an example of fixing that without breaking the current API.

@ManonMarchand
Copy link
Member

Hello,

I wanted to add a method to switch between the two simbad mirrors. How do I get the second one from the configuration of astropy? So far, I have this:

>>> from astroquery.simbad import conf
>>> next(conf.items())
('server', <ConfigItem: name='server' value='simbad.cds.unistra.fr' at 0x7f1b2c26a9d0>)

And I was wondering how I could access the url of the other mirror from that? Am I looking in the wrong place?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants