Skip to content

Conversation

@emileten
Copy link
Contributor

Solves #40 by adding a 'titiler-pgstac' construct, which mirrors the STAC API construct : I essentially

  • copy pasted the lib/stac-api folder content into a new lib/titiler-pgstac-api folder
  • removed the app.py code and simply wrote from titiler.pgstac.main import app in the handler.py
  • adapted the requirements and config.py (titiler.pgstac.main requires the postgres credentials to be set as environment variables)

@emileten emileten force-pushed the add-titiler-endpoint branch from 0dc16b9 to fac2b92 Compare May 23, 2023 03:13
@emileten emileten changed the title feat(titiler-pgstac-api): add titiler-pgstac endpoint Draft : feat(titiler-pgstac-api): add titiler-pgstac endpoint May 23, 2023
@emileten
Copy link
Contributor Author

emileten commented May 23, 2023

I tried to deploy, and unfortunately the size of the unzipped package is too large to be used for a lambda function (I think it's because titiler.pgstac has many dependencies), so I have to use a Dockerfile instead, just like it's done here or here (any thoughts @vincentsarago ? Thanks !)

Just pushed the Dockerfile, that works, but need to change the CDK code and test it, so I changed the title of this PR to Draft: *.

@vincentsarago
Copy link
Member

@emileten, Dockerimage in Lambda should only be use if absolutely no choice (because it's much slower, at least for dynamic tiling)

I think it's because titiler.pgstac has many dependencies

I've deployed titiler.pgstac in lambda before so I doubt this is the case!

Oh, I misread the comments at first. Yes using simple requirements.txt won't work with CDK. Using custom dockerfile to build the package is the way to go.

as you can see in the dockerfile we do https://github.com/developmentseed/cdk-pgstac/pull/42/files#diff-6b803bce2d18a876f97ed5ee61d6137408b36add96327274f990ff18d81430a4R11-R16 which is cleaning some of the dependencies (already in AWS lambda env). in eoAPI, we also specify --no-binary pydantic (https://github.com/developmentseed/eoAPI/blob/main/infrastructure/aws/dockerfiles/Dockerfile.raster#LL9C88-L9C108) which can also reduce the package size a bit

👍

@emileten
Copy link
Contributor Author

Great that matches with my understanding ! Thank you!

* added a new service deploying a titiler-pgstac service with access to the pgstac database, and exposing its API

change requirements and add Dockerfile

switch to docker build based lambda code to make the package lighter
@emileten emileten force-pushed the add-titiler-endpoint branch from 8f6faf4 to 5617b22 Compare May 24, 2023 05:46
@emileten emileten changed the title Draft : feat(titiler-pgstac-api): add titiler-pgstac endpoint feat(titiler-pgstac-api): add titiler-pgstac endpoint May 24, 2023
@emileten emileten added the enhancement New feature or request label May 26, 2023
@emileten emileten self-assigned this May 26, 2023
@emileten
Copy link
Contributor Author

Any chance someone takes a look in the following days?

@sharkinsspatial ? or anyone else 😄

Copy link
Member

@vincentsarago vincentsarago left a comment

Choose a reason for hiding this comment

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

LGTM but I'm not familiar enough with cdk-pgstac that you might want to wait for @alukach or @sharkinsspatial approval 🤷

Copy link
Member

@alukach alukach left a comment

Choose a reason for hiding this comment

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

Strongly recommended changes:

  1. git rm lib/titiler-pgstac-api/runtime/src/.env
  2. Don't write the pgstac secret values to stdout.

Otherwise, I think this looks good. Left some other comments but those are mostly focused on upping my understanding on how this work.

@emileten
Copy link
Contributor Author

emileten commented Jun 1, 2023

@alukach I added most of your suggestions -- good to merge ?

@alukach
Copy link
Member

alukach commented Jun 1, 2023

@alukach I added most of your suggestions -- good to merge ?

@emileten I agree with Vincent that injecting secrets as env variables at the CDK level would probably be simpler than doing so in the runtime of the code, but I'm good either way and think you can merge when you feel this is ready.

Copy link
Member

@alukach alukach left a comment

Choose a reason for hiding this comment

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

@emileten Realized I was still marked as "requested changes". I'm good with merging if you are.

Copy link
Member

@alukach alukach left a comment

Choose a reason for hiding this comment

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

@emileten Realized I was still marked as "requested changes". I'm good with merging if you are.

@emileten
Copy link
Contributor Author

emileten commented Jun 8, 2023

Thanks @alukach ! Should be -- just wanted to check the last comment I haven't read above tomorrow.

@emileten emileten merged commit a02acef into main Jun 9, 2023
@emileten emileten deleted the add-titiler-endpoint branch June 9, 2023 03:11
github-actions bot pushed a commit that referenced this pull request Jun 9, 2023
# [4.2.0](v4.1.0...v4.2.0) (2023-06-09)

### Features

* **titiler-pgstac-api:** add titiler-pgstac endpoint ([#42](#42)) ([a02acef](a02acef))
@emileten emileten mentioned this pull request Aug 30, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants