-
Notifications
You must be signed in to change notification settings - Fork 23
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
Feature/xyz routes #88
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the wait @iacopoff. I've played with the example notebooks and it looks very cool!
I left some minor comments.
The notebooks are very helpful, but I'm wondering if this repository is the right place for it. We should try to keep simple the management of the docs and the dependencies. Xpublish is front-end agnostic and FastAPI already provides good tools for documenting the API endpoints.
Hi @benbovy, I think I have opened this PR too early as I am kind of rethinking the design as I am understanding better xpublish and thinking also how to accommodate the future wmts router. However, it is still very helpful to get your comments at this stage. Regarding the notebooks, I can definitely delete them. The docstrings will be enough. I agree with you that a sort of factory class, such as There are 2 sets of parameters that are required by tiling and image production:
Regarding point 2. I am not sure whether you think that is outside the scope of this project. Thanks! |
xpublish/routers/xyz.py
Outdated
xyz_router = XYZRouter() | ||
|
||
|
||
@xyz_router.get("/tiles/{layer}/{time}/{z}/{x}/{y}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we could make less assumptions here about the structure of the dataset. I'd rather see something like /tiles/{var}/{z}/{x}/{y}
and allow setting time
or any other extra dimension(s) as a query parameter.
Allowing flexible image formats /tiles/{var}/{z}/{x}/{y}.{format}
would be great.
(If later xarray supports multiscale datasets pydata/xarray#4118 pydata/xarray#5376 it will be nice to have /tiles/{var}/{z}/{x}/{y}@{scale}x
too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we could make less assumptions here about the structure of the dataset. I'd rather see something like
/tiles/{var}/{z}/{x}/{y}
and allow settingtime
or any other extra dimension(s) as a query parameter.
yes it makes sense to be minimalist in the assumption about data structure.
In general the get_tile operation should be used by every tile services (XYZ, WMTS...). Different tile services can then implement a certain logic on how other dimensions (time included) can be managed.
These are some references about time dimension in WMTS:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benbovy which other image formats do you think it should support?
I started adding router factory classes in #89. Regarding data validation, I think that it would be better to expose the Regarding the color mapping parameters, I'm not sure to know why matplotlib would be required unless we want to support by default all colormaps available in matplotlib (I'd be OK with that). There is also colorcet on which datashader depends. |
It looks very promising @iacopoff! In fact, I'd like to base xarray-leaflet on xpublish when this PR gets in, as there is a lot of overlap. |
Thanks for chiming in @davidbrochart. I agree that there is quite some overlap with xarray-leaflet, at least for everything between an xarray dataset (it may be chunked data) and the created image tiles (projection / transform, mapping options, etc.). It would be great if somehow we could join efforts on this. Since xpublish is front-end agnostic it might make sense to implement this functionality here. In fact, I think we could probably pick up many things already implemented in xarray-leaflet since it is in a more advanced stage of development regarding the generation of the tiles. However, I'm not sure how easy / hard would be for xarray-leaflet to rely on xpublish for serving the tiles:
|
Yes I think it makes a lot of sense (at least for the default transformation), in this PR or in a follow-up PR. |
Alternatively, we could create one server per dataset. Not sure it makes sense to have multiple (many!) threads with each their own event loop, though. |
Hi @davidbrochart, thanks for your inputs!
The xyz service so far assumes the dataset projection == map projection, this is required for the tiling to work. In few words the user should take care of the reprojection outside xpublish. I thought this was good to reduce dependencies and to keep the code simple. Regarding chunking, the rioxarray reproject method persists the data in memory, so either you save the reprojected data to disk and then read it again in chunks or indeed it may not fit in memory if the dataset is large.
This is a nice functionality, I was thinking that Datashader also supports some of the transformation you probably are referring to by sampling the raster in The Datashader's raster. I would probably keep this development for the next PR, but it is good to think about it now.
Also to reply to @benbovy, I think that for a simple xyz service we don't need a colorbar. However, if we are going to develop a WMTS, then it would be good to add a colorbar and legend and it seems that datashader alone can't do that How can I get legends and colorbars for my Datashader plot?¶. So this may still be something for a later PR? @benbovy thanks for developing the router factory! I will have a look at it. |
This could be done in another PR I think. I'm not sure this would be really useful in most of uses cases, but if that makes sense for xarray-leaflet and/or other use cases we may add a |
Hi @benbovy, I have drafted the xyz-router-factory following #89. I think it works well particularly for setting optional parameters as you suggested. Now, I guess this PR will eventually go through after #89, right? |
It is possible to run a FastAPI application as an async task using uvicorn, since
Since the server will run in the Jupyter kernel as an async task, passing the transformation functions to the FastAPI app shouldn't be a problem, right? |
@davidbrochart, I was experimenting a bit trying finding ways for running uvicorn as non-blocking server and I came up with two options:
I am not familiar enough with async programming in Python, maybe you could tell if the second option is what you would need to make it work in I think it is possible to pass the transformation functions, they could be defined as class parameters, like in the xyz router factory class. |
Yes, the second option would be best. |
Hi @benbovy and @davidbrochart, in the last few days I have progressed a bit on the I think this current PR should be closed in favour of xyz-router-factory. There are two main improvements in this xyz service:
I have created a repo with some notebooks that show the usage of transformers (where I basically follow @davidbrochart's dynamic.ipynb ) and renders. Any feedback is much appreciated! thanks |
Hi @iacopoff, looks like https://github.com/iacopoff/xpublish-example/tree/main/notebooks doesn't exist. |
@davidbrochart ops, it was set to private, I have changed to public :) |
I looked at the |
@iacopoff this looks great indeed! I should find some time to finish #89, so that we can then merge your work on a XYZ router into the main branch! @davidbrochart |
So, the service is quite simple, however there are few design choices that may not be ideal: