Skip to content

Fix a few issues with generate_config - #10893

Merged
pllim merged 10 commits into
astropy:masterfrom
saimn:fix-generate-config
Oct 22, 2020
Merged

Fix a few issues with generate_config#10893
pllim merged 10 commits into
astropy:masterfrom
saimn:fix-generate-config

Conversation

@saimn

@saimn saimn commented Oct 18, 2020

Copy link
Copy Markdown
Contributor

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.

saimn added 5 commits October 18, 2020 13:09
@saimn
Fix generate_config when used for other packages
55573c6 
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.
@saimn
Filter all warnings
8963d4c
@saimn
Fix use of get_config
097ff8b
@saimn
Remove all output when importing modules
7809dc1
@saimn
Add changelog
797a4c9
@saimn saimn added the config label Oct 18, 2020
@saimn saimn added this to the v4.1.1 milestone Oct 18, 2020
@saimn
saimn requested review from bsipocz and pllim October 18, 2020 17:46
This was referenced Oct 18, 2020
Merged
Merged
@bsipocz

bsipocz commented Oct 18, 2020

Copy link
Copy Markdown
Member

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

saimn commented Oct 18, 2020

Copy link
Copy Markdown
Contributor Author

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

bsipocz commented Oct 18, 2020

Copy link
Copy Markdown
Member

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.

@saimn saimn mentioned this pull request Oct 18, 2020
Closed
@pllim pllim added the Bug label Oct 19, 2020
embray
embray reviewed Oct 19, 2020
View reviewed changes

@embray embray left a comment

Copy link
Copy Markdown
Member

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) ?

Comment thread astropy/config/configuration.py Resolved
pllim
pllim reviewed Oct 19, 2020
View reviewed changes
Comment thread astropy/config/configuration.py

from astropy.extern.configobj import configobj, validate
from astropy.utils import find_current_module
from astropy.utils import find_current_module, silence

@pllim pllim Oct 19, 2020

Copy link
Copy Markdown
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. 😱

astropy/astropy/utils/misc.py

Lines 62 to 64 in 079fcb7

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

Comment thread astropy/config/configuration.py Resolved
Comment thread astropy/config/configuration.py Resolved
Comment thread astropy/config/configuration.py Resolved
pllim
pllim reviewed Oct 20, 2020
View reviewed changes
Comment thread astropy/config/configuration.py Resolved
pllim
pllim reviewed Oct 20, 2020
View reviewed changes
Comment thread astropy/config/configuration.py Resolved
pllim
pllim approved these changes Oct 22, 2020
View reviewed changes

@pllim pllim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks!

@pllim

pllim commented Oct 22, 2020

Copy link
Copy Markdown
Member

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

@saimn

saimn commented Oct 22, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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

pllim commented Oct 22, 2020

Copy link
Copy Markdown
Member

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

Reviewers

@pllim pllim pllim approved these changes
@bsipocz bsipocz Awaiting requested review from bsipocz
+1 more reviewer
@embray embray embray left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Bug config

Projects

None yet

Milestone

v4.2

Development

Successfully merging this pull request may close these issues.

4 participants

@saimn @bsipocz @pllim @embray