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

[WIP] Support Windows #51

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

robintw
Copy link

@robintw robintw commented Jan 14, 2022

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
Copy link
Collaborator

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

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.

xarray-leaflet not working properly on Windows
2 participants