Ab/initial changes for mspc #1

abarciauskas-bgse · 2024-05-22T19:34:30Z

This PR primarily:

removes WMTS code + documentation
Adds planetary computer library and code to sign STAC asset urls

I made this a draft primarily because I'm still thinking what could be the best approach for conditionally signing requests with the planetary computer library.

Todos:

Address questions
Remove unused models (such as LayerDict)

Questions

Do we need titiler/stacapi/backend.py? I do not see how CustomSTACReader is being used anywhere.

I think the answer to the question above is we do need titiler/stacapi/backend.py if we're going to use the collections endpoint: https://github.com/developmentseed/titiler-stacapi-mspc/blob/main/titiler/stacapi/main.py#L87-L91 uses MosaicTilerFactory which is included in this repo and requires the STACAPIBackend which is defined in titiler/stacapi/backend.py and relies on CustomSTACReader (not sure how I missed this dependency before).

…eed/titiler-ms-pc-stacapi into ab/initial-changes-for-mspc

vincentsarago · 2024-05-23T07:22:40Z

docs/src/endpoints/items_endpoints.md

@@ -13,7 +13,6 @@ The `Item` endpoints are created using TiTiler's [MultiBaseTilerFactory](https:/
 | `POST` | `/collections/{collection_id}/items/{item_id}/statistics`                                                   | GeoJSON ([Statistics][multistats_geojson_model]) | return assets statistics for a GeoJSON (merged)
 | `GET`  | `/collections/{collection_id}/items/{item_id}/tiles[/{TileMatrixSetId}]/{z}/{x}/{y}[@{scale}x][.{format}]`  | image/bin                                        | create a web map tile image from assets
 | `GET`  | `/collections/{collection_id}/items/{item_id}[/{TileMatrixSetId}]/tilejson.json`                            | JSON ([TileJSON][tilejson_model])                | return a Mapbox TileJSON document
-| `GET`  | `/collections/{collection_id}/items/{item_id}[/{TileMatrixSetId}]/WMTSCapabilities.xml`                     | XML                                              | return OGC WMTS Get Capabilities


the items endpoints comes from titiler MultiBaseTilerFactory so it will still be present

change was reverted

vincentsarago · 2024-05-23T07:24:07Z

titiler/stacapi/settings.py

@@ -77,6 +77,7 @@ class STACAPISettings(BaseSettings):
    """STAC API settings"""

    stac_api_url: str
+    mspc_default_api_url: str = "https://planetarycomputer.microsoft.com/api/stac/v1"


I think it's fine having stac_api_url to default to https://planetarycomputer.microsoft.com/api/stac/v1

vincentsarago · 2024-05-23T07:30:08Z

titiler/stacapi/reader.py

+        # 2. Set a boolean in settings, something like "SIGN_REQUESTS"
+        # 3. Just assume all requests are to MS PC API and sign them. Stub out this function in tests and/or when ENV=test
+        if stac_api_config.stac_api_url == stac_api_config.mspc_default_api_url:
+            url = planetary_computer.sign(url)


FYI this is how it's done in MC planetary computer https://github.com/microsoft/planetary-computer-apis/blob/main/pctiler/pctiler/reader.py#L43-L59

to avoid having a new settings, we could simply check if the asset url is on MS datastore

FYI this is how it's done in MC planetary computer https://github.com/microsoft/planetary-computer-apis/blob/main/pctiler/pctiler/reader.py#L43-L59

Nice, thanks for sharing that. It looks like we would not use this code exactly because it relies on a CollectionConfigTable

to avoid having a new settings, we could simply check if the asset url is on MS datastore

👍🏽 it does appear we can re-use some of https://github.com/microsoft/planetary-computer-apis/blob/main/pccommon/pccommon/cdn.py#L8 to do this. I noticed that this code determines if there is an azureedge configured for the data, but I don't think there is anyway for this application to know about that? Since that code relies on (I think) some redis cache? https://github.com/microsoft/planetary-computer-apis/blob/main/pccommon/pccommon/config/core.py#L28

hrodmn

@abarciauskas-bgse I'm sorry I missed your request for a review - didn't have my notifications set up yet! That is fixed now.

We discussed this in our call today but one other way to sign the assets is to use the modifier parameter in the pystac.ItemSearch (or pystac.Client) class where we use it (backend.py, dependencies.py, factory.py I think)

ItemSearch(..., modifier=planetary_computer.sign_inplace)

I don't know if it would be more efficient than checking each asset url, but maybe it would be!

abarciauskas-bgse · 2024-05-24T01:42:31Z

@hrodmn I used the ItemSearch modifier option signing method. I verified it didn't break when non-MS PC results are returned from search. So this does seem cleaner than including a regex to check URLs against.

@vincentsarago are there any other initial changes you think we should make? For example, I am not sure if the models in models.py are being used apart from the Link model and I think they were used as part of the WMTS endpoints. Do you think it's safe to remove all of the models apart from Link?

vincentsarago · 2024-05-24T06:59:23Z

titiler/stacapi/backend.py

@@ -258,6 +259,7 @@ def get_assets(
            f"{self.url}/search",
            stac_io=stac_api_io,
            **params,
+            modifier=pc.sign_inplace,


vincentsarago · 2024-05-24T07:00:45Z

titiler/stacapi/settings.py

@@ -67,7 +68,7 @@ class RetrySettings(BaseSettings):
    retry_factor: Annotated[float, Field(ge=0.0)] = 0.0

    model_config = {
-        "env_prefix": "TITILER_STACAPI_API_",
+        "env_prefix": "TITILER_STACAPI_RETRY_",


why do we need this change?

some of these env_prefixs were the same, which seems like it could cause a bug if one of these classes both had the same attribute name so I thought we could rename the env_prefixes to match the class name. What do you think?

It's not necessary, pydantic basettings is smart enough to handle this.

they are distributed into multiple classes because they are not used at the same places

ok reverted those changes

vincentsarago · 2024-05-24T07:01:29Z

titiler/stacapi/stac_reader.py


 stac_config = STACSettings()
+stac_api_config = STACAPISettings()


This setting is not used?

thanks for noticing this, I had misconfigured the pre-commit linter so missed it!

vincentsarago · 2024-05-24T07:03:55Z

@abarciauskas-bgse yes, you can remove https://github.com/developmentseed/titiler-stacapi-mspc/blob/ab/initial-changes-for-mspc/titiler/stacapi/models.py#L88-L115

abarciauskas-bgse · 2024-05-24T16:09:39Z

@vincentsarago I moved this from draft to ready for review as I have addressed the outstanding questions I had. Let me know if you think any other changes should be made for this initial PR.

abarciauskas-bgse added 12 commits May 22, 2024 10:46

Modify readme

2da874d

Remove alternate URL and add planetary computer signing

5b86d9e

Remove WMTS

e0e4a1e

Remove application with auth from docs nav

56d2b0d

Revert changes to collections endpoints.md

bf1a1aa

Revert changes to collections endpoints.md

0672c74

Revert changes to items endpoints.md

31fd37a

remove wmts from collections_endpoints.md

cbeb23a

Remove WMTS from items_endpoints.md

96af83e

Revert change to alternate url

456683f

Revert change to alternate url

001059c

Merge branch 'ab/initial-changes-for-mspc' of github.com:developments…

b53de47

…eed/titiler-ms-pc-stacapi into ab/initial-changes-for-mspc

abarciauskas-bgse requested review from vincentsarago and hrodmn May 22, 2024 19:34

abarciauskas-bgse force-pushed the ab/initial-changes-for-mspc branch from 48682e6 to b53de47 Compare May 22, 2024 22:09

vincentsarago reviewed May 23, 2024

View reviewed changes

Revert change to items_endpoints.md

2913b23

hrodmn requested review from hrodmn and vincentsarago and removed request for hrodmn May 23, 2024 20:19

abarciauskas-bgse added 3 commits May 23, 2024 13:41

Reame stac_reader, add blob regex to settings, rewrite signing logic

343dd03

Add tests

726ef87

Add netcdf fixture json

6b4c619

hrodmn reviewed May 23, 2024

View reviewed changes

abarciauskas-bgse added 4 commits May 23, 2024 13:53

Fix import and add include_asset_types to test

3c0157c

Linting

6f25ae3

Add doc string

67e5df3

Use modifier in ItemSearch and not explicit signed_url

379ae6b

Remove blob regex

b0c1901

vincentsarago reviewed May 24, 2024

View reviewed changes

abarciauskas-bgse added 4 commits May 24, 2024 08:26

Fix unused import and linting

bbb3396

Remove unused models

6ec6ad4

Revert change to settings

79bcd7b

Remove unused fixture

2757a65

abarciauskas-bgse marked this pull request as ready for review May 24, 2024 15:39

abarciauskas-bgse added 2 commits May 24, 2024 08:51

Change repo name

ab31e93

Fix links in readme

8640e48

vincentsarago self-requested a review May 24, 2024 16:21

vincentsarago approved these changes May 24, 2024

View reviewed changes

abarciauskas-bgse merged commit 3b80678 into main May 24, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ab/initial changes for mspc #1

Ab/initial changes for mspc #1

abarciauskas-bgse commented May 22, 2024 •

edited

Loading

vincentsarago May 23, 2024

abarciauskas-bgse May 23, 2024

vincentsarago May 23, 2024

vincentsarago May 23, 2024

vincentsarago May 23, 2024

abarciauskas-bgse May 23, 2024

hrodmn left a comment •

edited

Loading

abarciauskas-bgse commented May 24, 2024

vincentsarago May 24, 2024

vincentsarago May 24, 2024

abarciauskas-bgse May 24, 2024

vincentsarago May 24, 2024

abarciauskas-bgse May 24, 2024

vincentsarago May 24, 2024

abarciauskas-bgse May 24, 2024

vincentsarago commented May 24, 2024

abarciauskas-bgse commented May 24, 2024


		stac_config = STACSettings()
		stac_api_config = STACAPISettings()

Ab/initial changes for mspc #1

Ab/initial changes for mspc #1

Conversation

abarciauskas-bgse commented May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrodmn left a comment • edited Loading

Choose a reason for hiding this comment

abarciauskas-bgse commented May 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincentsarago commented May 24, 2024

abarciauskas-bgse commented May 24, 2024

abarciauskas-bgse commented May 22, 2024 •

edited

Loading

hrodmn left a comment •

edited

Loading