xarray-contrib / xarray_leaflet

An xarray extension for tiled map plotting.
https://xarray-leaflet.readthedocs.io
MIT License
163 stars 21 forks source link

[WIP] Support Windows #51

Open robintw opened 2 years ago

robintw commented 2 years ago

Work in progress

Fixes #30.

This PR attempts to make xarray_leaflet work on Windows. There are a few fairly simple changes around paths etc that are needed to make it work on Windows, so I've done those. There are also some bigger changes that I've detailed below, which we may need to discuss.

At the moment I haven't tested this version on Linux or OS X, so I don't know which my Windows changes may have broken it on other platforms - I imagine there will need to be some alterations to make this work on all platforms.

So, on to the issues:

  1. The biggest issue I came across was that _start() wasn't being called, as the map_ready traitlet was never being set to True. This turned out to be some weird async issue where the following line in async_wait_for_bounds was hanging and never returning:
self.base_url = (await self.url_widget.get_url()).rstrip('/')

Commenting this out, and passing a get_base_url function in the original call to leaflet.plot (for example img.leaflet.plot(m, get_base_url=lambda x: "http://localhost:8888")) seems to work. I see that the code is going to quite a lot of effort to try and get the base URL, by using bits of the Jupyter widgets code. The authors of stackstac - which has a show function which provides similar functionality to xarray_leaflet, but only for dask-backed xarrays - also came across this problem, and solved it with a fairly simple regex as follows:

 def base_url_from_window_location(url: str):
        if not url:
            return None
        if path_re := re.match(r"(^.+?)\/(?:lab|notebook|voila)", url):
            return path_re.group(1)
        return None

(taken from here)

How would you feel about removing the URL widget based code, and switching to just using this as the default value for get_base_url?

  1. I found that ipyleaflet seemed to be very sensitive to how and when the _start() method was called, and sometimes the LocalTileLayer would be present in the list of layers, but wouldn't actually display on the map. I also had some confusion about the path and url parameters of LocalTileLayer - as sometimes only one was set, and sometimes both were set - even though the code doesn't seem to set the url parameter at all.

Therefore, I tried switching to use the TileLayer class instead - as we're actually using a URL anyway, so we don't need any of the 'local' bit of LocalTileLayer. This seemed to fix the problem and it might be a better approach, as it probably takes away the complexity of the 'local' bit.

  1. I found that when I had run pip install to install xarray_leaflet, it didn't seem to install and activate the jupyter server extension that is needed for xarray_leaflet to work. I'm not entirely sure why, as the setup.py seems to be following the instructions in the Jupyter documentation - but I'm wondering whether this is a platform issue too (though I'd have expected the Jupyter team to have dealt with that already).

So in summary: this branch works on Windows, but will need some more work to make it support all platforms. I've done a change from LocalTileLayer to TileLayer, and am proposing a change from the URL widget-based base_url calculation to a regex-based calculation.

What do you think? Is it worth me continuing with this PR?

davidbrochart commented 2 years ago

Thanks for looking into it @robintw! I need to look at your changes, and understand why you had to work around some issues.