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: titiler-pgstac v1 upgrade #398

Merged
merged 22 commits into from
Jul 25, 2024
Merged

feat: titiler-pgstac v1 upgrade #398

merged 22 commits into from
Jul 25, 2024

Conversation

smohiudd
Copy link
Contributor

@smohiudd smohiudd commented Jul 18, 2024

Upgrade to titiler-pgstac 1.3.0 using migration guide.

Updated raster API deployed to dev.openveda.cloud/api/raster/docs. Changes include:

  • rename mosaic/ endpoint to searches/
  • add new /collections/{collection_id} endpoint
  • add new /colorMaps endpoint
  • rename stac/ endpoint to /collections/{collection_id}/items/{item_id}
  • rename stac-alt/ endpoint to /alt/collections/{collection_id}/items/{item_id}

@smohiudd
Copy link
Contributor Author

@vincentsarago do we need to upgrade pgstac as well? We're currently running 0.7.10

@smohiudd smohiudd changed the title Titiler-pgstac v1 upgrade feat: titiler-pgstac v1 upgrade Jul 18, 2024
@vincentsarago
Copy link
Contributor

do we need to upgrade pgstac as well? We're currently running 0.7.10

no, I think you're fine

@vincentsarago
Copy link
Contributor

✨ looks good to me, did you use https://stac-utils.github.io/titiler-pgstac/latest/migrations/v1_migration/? was it useful?

@smohiudd
Copy link
Contributor Author

Yes very useful! I did notice a discrepancy in this section. It imports add_search_register_route and add_mosaic_register_route but uses:

add_search_register_route(
    app,
    # any dependency we want to validate
    # when creating the tilejson/map links
    tile_dependencies=[
        mosaic.layer_dependency,
        mosaic.dataset_dependency,
        mosaic.pixel_selection_dependency,
        mosaic.process_dependency,
        mosaic.rescale_dependency,
        mosaic.colormap_dependency,
        mosaic.render_dependency,
        mosaic.pgstac_dependency,
        mosaic.reader_dependency,
        mosaic.backend_dependency,
    ],
)
# add /list endpoint
add_search_list_route(app)

I'm assuming we have to import add_search_list_route instead?

@vincentsarago
Copy link
Contributor

vincentsarago commented Jul 18, 2024

@smohiudd so weird the documentation web site doesn't point to the write .md files 😭

☝️ ignore, this was fixed in stac-utils/titiler-pgstac@6d84641

@smohiudd
Copy link
Contributor Author

When running the raster api docker compose I got this error: titiler-pgstac 1.3.0 depends on titiler.core<0.19 and >=0.18.0. So upgrading titiler core/mosaic/extensions to >=0.18.5,<0.19

@vincentsarago vincentsarago force-pushed the feature/pgstac-v1-upgrade branch from 9572a73 to fcf3fed Compare July 19, 2024 16:25
@smohiudd smohiudd marked this pull request as ready for review July 19, 2024 16:51
@smohiudd smohiudd requested a review from anayeaye July 19, 2024 17:48
@anayeaye
Copy link
Collaborator

@smohiudd I think we also need to replace/remove VEDA_RASTER_ENABLE_MOSAIC_SEARCH from docker-compose

@slesaad
Copy link
Member

slesaad commented Jul 22, 2024

@smohiudd we also wanted to add the colormap endpoints - should we do it in the same PR?

@@ -67,24 +78,36 @@ async def lifespan(app: FastAPI):
# /mosaic - PgSTAC Mosaic titiler endpoint
###############################################################################
mosaic = MosaicTilerFactory(
router_prefix="/mosaic",
router_prefix="/searches/{search_id}",
path_dependency=SearchIdParams,
optional_headers=optional_headers,
environment_dependency=settings.get_gdal_config,
process_dependency=PostProcessParams,
Copy link
Contributor Author

@smohiudd smohiudd Jul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need process_dependency if we have tile_dependencies below?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smohiudd the process dependency is different from the tile_dependency. I see that we have custom post_process method, so if they are still used we should keep the process_dependency

@smohiudd
Copy link
Contributor Author

@vincentsarago how come we don't have the /collections/{collection_id} endpoint: https://dev.openveda.cloud/api/raster/docs

@anayeaye
Copy link
Collaborator

Are these deprecated searches visible in the swagger docs as a courtesy to inform users that users must now supply {TileMatrixSetId} instead of expecting a default web mercator?

deprecated-endpoints

# TODO
# prefix will be replaced by `/mosaics/{search_id}` in titiler-pgstac 1.0.0
# add /list endpoint
add_search_list_route(app, prefix="/searches", tags=["Mosaic"])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the equivalent of the list registered mosaics endpoint that we were setting in the .env as VEDA_RASTER_ENABLE_MOSAIC_SEARCH and then

  1. Inserting in the lambda env in infrastructure construct

  2. And using the runtime config to conditionally enable the list endpoint which could potentially be expensive because it is not paginated (or it was not yet paginated at some point)?

If so, do we want it always on now?

@vincentsarago
Copy link
Contributor

@smohiudd

@vincentsarago how come we don't have the /collections/{collection_id} endpoint: https://dev.openveda.cloud/api/raster/docs

you need to register a new set of endpoint using the MosaicTilerFactory factory

https://github.com/stac-utils/titiler-pgstac/blob/6d84641beda8ab28a79a1864d7700b30926c141a/titiler/pgstac/main.py#L172-L188

@vincentsarago
Copy link
Contributor

Are these deprecated searches visible in the swagger docs as a courtesy to inform users that users must now supply {TileMatrixSetId} instead of expecting a default web mercator?

@anayeaye, yes! in next major version of titiler we will remove the default tms and thus the user will always have to pass a TileMatrixSetId. This is to match the OGC Tiles specification

# TODO
# prefix will be replaced by `/mosaics/{search_id}` in titiler-pgstac 1.0.0
# add /list endpoint
add_search_list_route(app, prefix="/searches", tags=["Mosaic"])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the tags should be Searches ?

)
app.include_router(mosaic.router, prefix="/searches/{search_id}", tags=["Mosaic"])

add_search_register_route(
Copy link
Contributor

@vincentsarago vincentsarago Jul 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add

if settings.enable_mosaic_search:
    add_search_register_route(

app.include_router(
collection.router, tags=["STAC Collection"], prefix="/collections/{collection_id}"
)


###############################################################################
# /stac - Custom STAC titiler endpoint
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from titiler.pgstac.dependencies import ItemIdParams

stac = MultiBaseTilerFactory(
    reader=PgSTACReader,
    path_dependency=ItemIdParams,
    optional_headers=optional_headers,
    router_prefix="/stac",
    environment_dependency=settings.get_gdal_config,
    router=APIRouter(route_class=LoggerRouteHandler),
    extensions=[
        stacViewerExtension(),
    ],
    colormap_dependency=ColorMapParams,
)
app.include_router(stac.router, tags=["Items"], prefix="/collections/{collection_id}/items/{item_id}")

###############################################################################
# /alt - Custom STAC titiler endpoint for alternate asset locations
###############################################################################
stac_alt = MultiBaseTilerFactory(
    reader=PgSTACReaderAlt,
    path_dependency=ItemIdParams,
    optional_headers=optional_headers,
    router_prefix="/alt/collections/{collection_id}/items/{item_id}",
    environment_dependency=settings.get_gdal_config,
    router=APIRouter(route_class=LoggerRouteHandler),
    extensions=[
        stacViewerExtension(),
    ],
    colormap_dependency=ColorMapParams,
)
app.include_router(stac_alt.router, tags=["Items"], prefix="/alt/collections/{collection_id}/items/{item_id}")

@smohiudd smohiudd requested review from anayeaye and slesaad July 24, 2024 19:38
@smohiudd smohiudd merged commit f449325 into develop Jul 25, 2024
4 checks passed
@smohiudd smohiudd deleted the feature/pgstac-v1-upgrade branch July 25, 2024 15:27
botanical added a commit that referenced this pull request Aug 1, 2024
…configuration for cloudfront, titiler-pgstac v1 upgrade (#405)

### 📣 Breaking  
- [feat!: titiler-pgstac v1 upgrade
#398](#398) introduces
breaking endpoint changes; see [titiler-pgstac v0.8 -> v1.0 migration
docs](https://stac-utils.github.io/titiler-pgstac/latest/migrations/v1_migration/)

### ✨  Added
- [feat: add optional web acl configuration for cloudfront
#396](#396)

### 🪙  Changed/Updated
- [chore: Improve stac integration tests
#395](#395)
- [chore: add gitignore entry for Jetbrains IDEs
#409](#409)


### 🩹 Fixed
- [fix: render fix for titiler-pgstac v1 upgrade
#408](#408)
sandrahoang686 added a commit to NASA-IMPACT/veda-ui that referenced this pull request Aug 5, 2024
…1075)

**Related Ticket:** #1070

⚠️ **This needs to be coordinated with veda BE team on when this can be
merged. BE team needs to merge their changes into their prod before this
can merge.**

### Description of Changes
Updating raster api (titiler) endpoints as part of new version breaking
changes. This does not worry about integrating new endpoints.

### Notes & Questions About Changes
* [VEDA BACKEND
PR](NASA-IMPACT/veda-backend#398) for ref
* [Titiler version migration
guide](https://stac-utils.github.io/titiler-pgstac/latest/migrations/v1_migration/#searchid-id)
for ref

### Relevant API Changes
[**yes**] rename mosaic/ endpoint to searches/
[**no**] for `/register` endpoints, update `searchId` to `id`
[**no**] a dd new /collections/{collection_id} endpoint
[**no**] add new /colorMaps endpoint
[**no**] rename stac/ endpoint to
/collections/{collection_id}/items/{item_id}
[**no**] rename stac-alt/ endpoint to
/alt/collections/{collection_id}/items/{item_id}

### Validation / Testing
_{Update with info on what can be manually validated in the Deploy
Preview link for example "Validate style updates to selection modal do
NOT affect cards"}_
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.

4 participants