Closed Carifio24 closed 6 months ago
It looks like there's a pin in the CI (pin-1
) that's requiring Python 3.12(?). I don't understand why conda would use Python 3.12 when we set the Python version here, though.
@pkgw Ok, I think I have a beat on this now. So while we specify our Python version where I had linked above, the offending step is actually before that, in the setup. There we're installing brotlipy
using the base environment (the comment says it's for a Windows workaround), which is now using Python 3.12 by default.
According to the runner info, it's using Miniconda 24.3.0. Miniconda added support for Python 3.12 in 24.1.2. The last CI prior to that was using Miniconda 23.10.0, so no Python 3.12, and thus not surprising that we didn't see this issue.
I'll admit I don't know exactly what we use brotlipy
for, as it only seems to be referenced in that part of the CI. The Brotli project itself offers a brotli
Python package, whereas brotlipy
seems to have been become/been replaced by brotlicffi
. Maybe it's time to upgrade?
We wouldn't use Brotli(py) directly. In my own system, I see that urllib3 depends on it, though, so it probably comes in as a transitive dependency somewhere in the web stack (which makes sense, since Brotli is used as a compression format by modern web servers).
I'm afraid that I don't remember exactly what was going on that inspired me to add in the install step — probably some kind of missing library or something that prevented Conda from running. I've just pushed a commit to just remove the install since it was just a workaround to begin with.
Removing the brotlipy
install seems to have fixed this issue! My most recent commit deals with the actual test failures. The issues were:
pytest.warns
now requires a str
or a Warning
input - we're currently passing it None
. I modified that use warnings.catch_warnings
, which is the recommended approach here.qt_full_step4
on Linux seem the same to me - as in, there's obviously a nonzero diff but I can't see a difference with the eye test. However, the new MacOS sky_layers
image actually IS different, because you can see the background tiles. @pkgw is this okay? I was wondering if maybe this is something where a runner improvement made it so that the tiles actually load in when the image is rendered. I've added the new image in this commit, but I'll remove it if this is actually not expected behavior.Attention: Patch coverage is 50.00000%
with 1 lines
in your changes are missing coverage. Please review.
Project coverage is 56.51%. Comparing base (
2c7377b
) to head (5ec4686
).:exclamation: Current head 5ec4686 differs from pull request most recent head e1c6431. Consider uploading reports for the commit e1c6431 to get more accurate results
Files | Patch % | Lines |
---|---|---|
pywwt/core.py | 50.00% | 1 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
The test that involves the sky_layers
image starts by setting the foreground and background to the "Black Sky Background" imagery (here), so it's incorrect for stars to be showing up in the test image. It's not obvious to me why the behavior would be changing here. Clearly this PR shouldn't affect that, and I can't think of other recent changes that would either.
To my eye, the other image changes seem to be stemming from the background map not having tiled in to its full resolution just yet. Fetching those tiles is something that should be pretty reliable. It's possible that this could be worked around by increasing the delays in the tests, to give the engine more time to load images, but it might be good to double-check that this isn't indicating that our webservices are having some kind of issue.
This PR exposes the cache-refreshing functionality of the research app to pywwt. We have a use case for this in CosmicDS (not urgent, but it was easy so I figured I'd get the ball rolling), but I think having the option available is generally useful. I did add a cautionary note in my docstring that this should really only be used when it's needed, though.
If we want a test, I'm not really sure what a good one would be - I don't see a straightforward way to use the image comparison method here.