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

feat: use localtileserver as native deps #835

Merged
merged 6 commits into from
Jun 9, 2023
Merged

feat: use localtileserver as native deps #835

merged 6 commits into from
Jun 9, 2023

Conversation

12rambau
Copy link
Member

@12rambau 12rambau commented Jun 6, 2023

people from kitware and I we finally finished the full rasterio implementation of lagre_image (girder/large_image#1115) which allowed localtileserver to now only depends on rasterio (https://github.com/banesullivan/localtileserver/releases/tag/0.7.0). That's a very good news as it means local images can finally be shown in interactive maps within sepal-ui applications.

In this PR:

  • I used localtileserver as a default dependency (remove any traces of lazy imports)
  • I edited a bit the docstring for special cases (SEPAL being one of them)

for @dfguerrerom:
As SEPAL fall in the "distant jupyter" know dificulties (https://localtileserver.banesullivan.com/installation/remote-jupyter.html), I also imported a proxy generator in the lib. If I'm not wrong you should be able to make everything work by setting the LOCALTILESERVER_CLIENT_PREFIX env variable to "/api/sandbox/jupyter/proxy/{port}". I think you will need to work with @cdanielw to add this env variable in every user instances (e.g. in the default .bashrc) so that user get something on their screen without the need to go to their terminal.

Let me know if you think I should modify anything, I'm planning on merging it next week.

@codecov
Copy link

codecov bot commented Jun 6, 2023

Codecov Report

Merging #835 (14adda6) into main (9e2335d) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #835      +/-   ##
==========================================
+ Coverage   96.52%   96.57%   +0.05%     
==========================================
  Files          39       39              
  Lines        3767     3764       -3     
==========================================
- Hits         3636     3635       -1     
+ Misses        131      129       -2     
Impacted Files Coverage Δ
sepal_ui/mapping/sepal_map.py 90.90% <100.00%> (+0.51%) ⬆️

@dfguerrerom
Copy link
Collaborator

Thanks for that @12rambau. I have tested it on SEPAL and it works smoothly.
Is it normal that the raster is lost when you zoom more than 12? On my end it totally disappears, I have tried it using two different images in two different envs.

@12rambau
Copy link
Member Author

12rambau commented Jun 8, 2023

That's unexpected, is there a max_zoom member to the created layer ?

@dfguerrerom
Copy link
Collaborator

Localtilelayer sets max_zoom as the raster default_zoom, but when it comes to satellite imagery, users prefer to zoom at the pixel level. I have hardcoded 20 as max_zoom level taking into account that the max zoom of google satellite layers ranges from 18-21, what do you think?

@12rambau
Copy link
Member Author

12rambau commented Jun 9, 2023

Thanks for finding out, I would go directly to the max value of leaflet basemaps (24) let me update the value and fix the test and I'll request your review again

@12rambau
Copy link
Member Author

12rambau commented Jun 9, 2023

It is also forcing me to drop support of Python 3.7 as it's not supported by large_image any more. That's ok as py3.7 is reaching EOL on 2023-06-27

@12rambau 12rambau requested a review from dfguerrerom June 9, 2023 11:45
@12rambau 12rambau merged commit bbf7869 into main Jun 9, 2023
@12rambau 12rambau deleted the localtileserver branch June 9, 2023 14:05
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