Details
-
Type:
Bug
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: meas_mosaic, validate_drp
-
Labels:None
-
Story Points:0.5
-
Epic Link:
-
Team:SQuaRE
Description
meas.mosaic.utils does a module-level import of matplotlib and sets the backend to Agg.
a) Specifically the matplotlib.use("Agg") line should be removed.
It has no effect if the user has already loaded a backend, and spews a big chunk of distracting warnings.
b) The overall import of matplotlib should not happen at the module level, because that code gets run when importing the module. Importing the module happens even just opening a Butler object view to the repo. Specifically, meas_mosaic gets imported by just the following code.
import os
|
import lsst.afw.display as afwDisplay
|
from lsst.daf.persistence import Butler
|
|
|
repo = os.getenv('REPO')
|
butler = Butler(repo)
|
Philosophically I don't believe that opening a Butler should trigger setting up matplotlib.
Editorial: I believe that all plotting imports should be protected and separated from the data analysis. But this implies a refactoring that may not be justified given the imminent replacement by jointcal
Attachments
Issue Links
- is triggering
-
DM-15429 Assertion failure in meas_mosaic diagnostics
- Done
Activity
Michael Wood-Vasey
created issue -
Michael Wood-Vasey
made changes -
Michael Wood-Vasey
made changes -
Michael Wood-Vasey
made changes -
| Link | This issue relates to IHS-581 [ IHS-581 ] |
Michael Wood-Vasey
made changes -
| Risk Score | 0 |
Possibly here?
Paul Price
added a comment - Possibly here ? +1
Michael Wood-Vasey
added a comment - +1 | Description |
`meas.mosaic.utils` does a module-level import of `matplotlib` and sets the backend to `Agg`.
a) Specifically the `matplotlib.use("Agg")` line should be removed. [https://github.com/lsst/meas_mosaic/blob/master/python/lsst/meas/mosaic/utils.py#L30]It has no effect if the user has already loaded a backend, and spews a big chunk of distracting warnings. b) The overall import of matplotlib should not happen at the module level, because that code gets run when importing the module. Importing the module happens even just opening a Butler object view to the repo. Specifically, `meas_mosaic` gets imported by just the following code. ``` import os import lsst.afw.display as afwDisplay from lsst.daf.persistence import Butler repo = os.getenv('REPO') butler = Butler(repo) ``` Philosophically I don't believe that opening a Butler should trigger setting up `matplotlib`. Editorial: I believe that all plotting imports should be protected and separated from the data analysis. But this implies a refactoring that may not be justified given the imminent replacement by `jointcal` |
{{meas.mosaic.utils}} does a module-level import of matplotlib and sets the backend to {{Agg}}.
a) Specifically the [{{matplotlib.use("Agg")}}|https://github.com/lsst/meas_mosaic/blob/master/python/lsst/meas/mosaic/utils.py#L30] line should be removed. It has no effect if the user has already loaded a backend, and spews a big chunk of distracting warnings. b) The overall import of matplotlib should not happen at the module level, because that code gets run when importing the module. Importing the module happens even just opening a Butler object view to the repo. Specifically, meas_mosaic gets imported by just the following code. {code} import os import lsst.afw.display as afwDisplay from lsst.daf.persistence import Butler repo = os.getenv('REPO') butler = Butler(repo) {code} Philosophically I don't believe that opening a Butler should trigger setting up matplotlib. Editorial: I believe that all plotting imports should be protected and separated from the data analysis. But this implies a refactoring that may not be justified given the imminent replacement by jointcal |
| Team | Data Release Production [ 10301 ] | |
| Summary | Wrap `matplotlib` use in `meas_mosaic` | Wrap matplotlib use in meas_mosaic |
| Priority | Minor [ 4 ] | Critical [ 2 ] |
Other code that uses matplotlib on machines without X (e.g. the worker nodes) falls over because of this.
Basically, because one ends up not being able to set the backend (because the matplotlib.use('Agg') call fails to have an effect) any calls to make figures or plot things results in it trying to contact the display server and failing (with an ugly an non-obvious exit).
For others suffering, a temporary workaround is to put
backend : Agg
in your matplotlibrc file.
See instructions here for how to do so:
https://matplotlib.org/users/customizing.html#the-matplotlibrc-file
If the above worked for you, then it's not the meas_mosaic matplotlib.use statement in the module import that's causing you problems, it must be something else that's setting the matplotlib backend to a default that's not available on the worker nodes.
Michael Wood-Vasey
added a comment - Merlin Fisher-Levine
If the above worked for you, then it's not the meas_mosaic matplotlib.use statement in the module import that's causing you problems, it must be something else that's setting the matplotlib backend to a default that's not available on the worker nodes. Michael Wood-Vasey
made changes -
Michael Wood-Vasey
made changes -
Paul Price We really shouldn't have meas_mosaic dependencies in pipe_tasks, particularly ones that aren't captured in the table file. What is that doing there?
How do you expect us to use it if we can't use it?
Paul Price
added a comment - How do you expect us to use it if we can't use it? Isn't pipe_tasks below meas_mosaic in the stack hierarchy? This seems like a recipe for circular dependencies.
To actually answer your question: code that does the "apply wcs/photoCalib" step (in this case applyMosaicResults) should just live in pipe_tasks or even lower down (pipe_base?). It shouldn't have any dependency on meas_mosaic or jointcal, and it should cleanly handle the non-existence of those datasets by doing a no-op.
This specific issue addressed by this ticket doesn't cause any significant problems. It's just annoying to see the warning.
Michael Wood-Vasey
added a comment - This specific issue addressed by this ticket doesn't cause any significant problems. It's just annoying to see the warning. I don't believe the fix on the ticket branch will solve the related problem here, which is that if this code was imported one would still not be able to set the matplotlib backend because of the subsequent import matplotlib.pyplot as plt.
Good point. Considering.
Michael Wood-Vasey
added a comment - Good point. Considering. Michael Wood-Vasey
made changes -
| Priority | Critical [ 2 ] | Minor [ 4 ] |
After some discussion with John Parejko, it became clear that the correct way to solve this for validateDrp.py example runs would be to specify the backend in a current-working directory level matplotlibrc file. This has the benefit of removing hard-coded lines that might not even work and making it more clear where this is happening for the runs that we'd like to distribute to displayless nodes.
Michael Wood-Vasey
added a comment - After some discussion with John Parejko , it became clear that the correct way to solve this for validateDrp.py example runs would be to specify the backend in a current-working directory level matplotlibrc file. This has the benefit of removing hard-coded lines that might not even work and making it more clear where this is happening for the runs that we'd like to distribute to displayless nodes. After further discussion it may be that putting these lines in bin.src/validateDrp.py is a better and more clearer solution.
Michael Wood-Vasey
added a comment - - edited After further discussion it may be that putting these lines in bin.src/validateDrp.py is a better and more clearer solution. Michael Wood-Vasey
made changes -
| Assignee | Michael Wood-Vasey [ wmwood-vasey ] |
Michael Wood-Vasey
made changes -
| Story Points | 0.25 |
Michael Wood-Vasey
made changes -
| Reviewers | John Parejko, Simon Krughoff [ parejkoj, krughoff ] | |
| Status | To Do [ 10001 ] | In Review [ 10004 ] |
One comment about your comment, otherwise you're good to go.
| Status | In Review [ 10004 ] | Reviewed [ 10101 ] |
Michael Wood-Vasey
made changes -
| Story Points | 0.25 | 0.5 |
Michael Wood-Vasey
made changes -
| Component/s | validate_drp [ 14011 ] |
Merged to master.
Michael Wood-Vasey
added a comment - Merged to master. Michael Wood-Vasey
made changes -
| Resolution | Done [ 10000 ] | |
| Status | Reviewed [ 10101 ] | Done [ 10002 ] |
Michael Wood-Vasey
made changes -
| Epic Link |
|
Michael Wood-Vasey
made changes -
| Team | Data Release Production [ 10301 ] | SQuaRE [ 10302 ] |
Paul Price
made changes -
And to clarify, what really surprised me about this is that I'm not consciously using meas_mosaic and I can't find where it gets picked up. Why does it get imported anyway?