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

Allow refreshing tile cache #375

Merged
merged 10 commits into from
May 16, 2024

Conversation

Carifio24
Copy link
Member

@Carifio24 Carifio24 commented Apr 18, 2024

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.

@Carifio24
Copy link
Member Author

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 pkgw closed this Apr 30, 2024
@pkgw pkgw reopened this Apr 30, 2024
@Carifio24
Copy link
Member Author

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

As seen in WorldWideTelescope#375, the workaround added in WorldWideTelescope#352 may be causing problems
now. I don't recall exactly what the issue was before, but we don't
use brotli, so as a first step let's just get rid of the install
command.
@pkgw
Copy link
Contributor

pkgw commented May 1, 2024

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.

@Carifio24
Copy link
Member Author

Carifio24 commented May 2, 2024

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.
  • Image test corpus updates. The new images for 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.

Copy link

codecov bot commented May 3, 2024

Codecov Report

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

❗ 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 ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #375      +/-   ##
==========================================
- Coverage   56.51%   56.51%   -0.01%     
==========================================
  Files          25       25              
  Lines        2870     2872       +2     
==========================================
+ Hits         1622     1623       +1     
- Misses       1248     1249       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pkgw
Copy link
Contributor

pkgw commented May 6, 2024

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.

@Carifio24 Carifio24 closed this May 7, 2024
@Carifio24 Carifio24 reopened this May 7, 2024
@Carifio24 Carifio24 closed this May 7, 2024
@Carifio24 Carifio24 reopened this May 7, 2024
@Carifio24 Carifio24 closed this May 8, 2024
@Carifio24 Carifio24 reopened this May 8, 2024
@pkgw pkgw merged commit 0f6d3f0 into WorldWideTelescope:master May 16, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants