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

Fix a few issues with generate_config #10893

Merged
merged 10 commits into from Oct 22, 2020
Merged

Conversation

saimn
Copy link
Contributor

@saimn saimn commented Oct 18, 2020

When using it with other packages (tried this with astroquery), this fixes several issues:

  • Astropy modules where imported and then added to the config file.
  • One module (astroquery.mpc) was added 3 times because it is imported in other modules.
  • Astroquery instantiates its classes at import which causes a lot of warnings/logging messages, so I choose to silence all outputs.

When using it with astroquery, this fixes 2 issues:
- Astropy modules where imported and then added to the config file.
- One module (`astroquery.mpc`) was added 3 times because it is imported
  in other modules.
@bsipocz
Copy link
Member

bsipocz commented Oct 18, 2020

One module (astroquery.mpc) was added 3 times because it is imported in other modules.

what do you mean exactly? It's imported into the pseudo module solarsystem, is that causing troubles? (what I don't understand that the same is done for the jpl modules, do those behave differently?)

Astroquery instantiates its classes at import which causes a lot of warnings/logging messages, so I choose to silence all outputs.

if there are obsessive logging/warnings, we can certainly look into that and do a cull. Could you provide examples?

@saimn
Copy link
Contributor Author

saimn commented Oct 18, 2020

what do you mean exactly? It's imported into the pseudo module solarsystem, is that causing troubles? (what I don't understand that the same is done for the jpl modules, do those behave differently?)

Yes, it happens when importing solarsystem.mpc for example, which creates a new mpc.Conf class. I think it happens because importing mpc fails because of astropy._erfa.core, and so the module import is not cached. And I just realized that I had updated astroquery but I had an old version installed in my site-packages, so with astroquery master the error is gone and it works correctly, without the multiple conf classes. Anyway I think it's still reasonable to have this change to be safe.

if there are obsessive logging/warnings, we can certainly look into that and do a cull. Could you provide examples?

Sure, this is only when filtering only deprecation warnings: https://gist.github.com/saimn/a18203a6ca86e6c573af0b53fb9b0d37

@bsipocz
Copy link
Member

bsipocz commented Oct 18, 2020

Glad to hear that master is being fixed, I recall a few back and forth of how to deal with those erfa things.

Sure, this is only when filtering only deprecation warnings: https://gist.github.com/saimn/a18203a6ca86e6c573af0b53fb9b0d37

Ahh, thanks. We should raise errors for deprecations, so if any of those are around that's a real issue (with the exception of deprecations coming from pyregion or html5lib). Could you report them back in an astroquery issue?

Most of those are legit warnings, and the rest is already on the radar to be cleaned up.

Copy link
Member

@embray embray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While making changes in this function anyways, how does my idea in this comment sound #10877 (comment) ?

astropy/config/configuration.py Outdated Show resolved Hide resolved
@@ -23,7 +23,7 @@
from contextlib import contextmanager

from astropy.extern.configobj import configobj, validate
from astropy.utils import find_current_module
from astropy.utils import find_current_module, silence
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these years, I didn't know this existed. 😱

@contextlib.contextmanager
def silence():
"""A context manager that silences sys.stdout and sys.stderr."""

astropy/config/configuration.py Outdated Show resolved Hide resolved
astropy/config/configuration.py Show resolved Hide resolved
astropy/config/configuration.py Outdated Show resolved Hide resolved
Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@pllim
Copy link
Member

pllim commented Oct 22, 2020

@saimn , perhaps you should answer @embray 's question about #10877 (comment) before merge?

@saimn
Copy link
Contributor Author

saimn commented Oct 22, 2020

@pllim - I think I did so ?
I mean, @embray proposed a slightly different solution, but we can discuss that in #10877.

@pllim pllim merged commit d976f8d into astropy:master Oct 22, 2020
@pllim
Copy link
Member

pllim commented Oct 22, 2020

Okay then, let's continue discussion on the other issue. Thanks, everyone!

@saimn saimn deleted the fix-generate-config branch October 22, 2020 15:32
@bsipocz bsipocz modified the milestones: v4.1.1, v4.2 Nov 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants