Fix prettysummary import in CloudMicrophysics extension#195
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| ##### | ||
|
|
||
| import Oceananigans.Utils: prettysummary | ||
| import Oceananigans.Grids: prettysummary |
There was a problem hiding this comment.
I think prettysummary is originally defined in Utils but then probably is extended in Grids
There was a problem hiding this comment.
Does it matter where we import it?
There was a problem hiding this comment.
I think prettysummary is originally defined in Utils but then probably is extended in Grids
No, that's the thing, Utils imports it from Grids: https://github.com/CliMA/Oceananigans.jl/blob/81dc6c5db02ba111ffc750999e3ca83402dfebe3/src/Utils/prettysummary.jl#L1
Does it matter where we import it?
Yes, ExplicitImports checks that, and tests are failing locally for me because of this.
There was a problem hiding this comment.
It might be a messiness / entanglement issue wherein Grids precedes Utils in Oceananigans
There was a problem hiding this comment.
Do you think it's worth inverting the imports in Oceananigans?
There was a problem hiding this comment.
I don't think it's wrong, I was just amazed that it mattered... (but I had forgotten about ExplicitImports)
There was a problem hiding this comment.
Do you think it's worth inverting the imports in Oceananigans?
We should look at it. I do think we should be continuously pushing to resolve Oceananigans technical debt.
There was a problem hiding this comment.
Ooof, I did look into it, but defining Utils before Grids is a mess, because Utils defines methods on Grids types, for example https://github.com/CliMA/Oceananigans.jl/blob/81dc6c5db02ba111ffc750999e3ca83402dfebe3/src/Utils/kernel_launching.jl#L146 (Face is defined in https://github.com/CliMA/Oceananigans.jl/blob/81dc6c5db02ba111ffc750999e3ca83402dfebe3/src/Grids/Grids.jl#L54)
There was a problem hiding this comment.
Thinking harder about this, the methods in Utils using types in Grids could just be moved to Grids.
No, that's very weird... smells like a Julia bug to me but not sure. File an ExplicitImports issue? I might be able to investigate later |
#201) * Always load `CloudMicrophysics` when running the quality assurance tests This enables `ExplicitImports` to consistently analyse the code of the extension. * DROP ME: Revert "Fix `prettysummary` import in CloudMicrophysics extension (#195)" This reverts commit 5ffe5fd. * Revert "DROP ME: Revert "Fix `prettysummary` import in CloudMicrophysics extension (#195)"" This reverts commit 14167b0.
4 participants
I'm a bit confused by why this doesn't trigger an error systematically (@ericphanson ideas?), but this has been failing in some jobs on
main(e.g. https://github.com/NumericalEarth/Breeze.jl/actions/runs/19600740942/job/56131898424#step:6:756) and also locally for me (but I'm using julia v1.12 on macOS, same combination as the job linked above).Honestly, I'm tempted by fixing this upstream in Oceananigans, it feels like
prettysummaryshould really have been defined inUtils, notGrids.