Skip to content

Conversation

@vincentsarago
Copy link
Contributor

Description

This PR update TiTiler-PgSTAC requirement (+titiler and rio-tiler)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

TODO

Locally, just made

Checklist:

Please delete options that are not relevant.

  • I have performed a self-review
  • Changelog has been updated
  • Documentation has been updated
  • Unit tests pass locally (./scripts/test)
  • Code is linted and styled (./scripts/format)

@ghost
Copy link

ghost commented Dec 22, 2021

CLA assistant check
All CLA requirements met.

################################################################################
registered_cmaps = cmap
custom_colormaps: Dict[str, Dict[int, List[int]]] = {
custom_colormaps: Dict[str, ColorMapType] = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

update to use new rio-tiler ColorMap Type

98: [10, 5, 250, 255],
99: [5, 2, 252, 255],
100: [0, 0, 255, 255],
0: (255, 255, 255, 255),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Type, is now a Tuple

path_dependency=ItemPathParams,
colormap_dependency=PCColorMapParams,
router_prefix=get_settings().item_endpoint_prefix,
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no more need to create a custom TilerFactory, we can use MultiBaseTilerFactory and set custom params

f"collections/{collection}/items/{item}",
)

def register_poly_crop(self) -> None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

/crop is now in titiler tiler factory

raise Exception("Cannot make a colormap from Intervals colormap")

if len(cm) > 256 or max(cm) >= 256:
raise Exception("Cannot make a colormap for discrete colormap")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

make_lut can only work with GDAL like colormap

@dataclass
class AssetsBidxExprParams(dependencies.AssetsBidxExprParams):

collection: str = Query(None, description="STAC Collection ID")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we add collection to AssetsBidxExprParams params which will be used in all endpoints

reader=CustomPGSTACBackend, # type:ignore
router_prefix=get_settings().mosaic_endpoint_prefix,
pgstac_mosaic_factory = MosaicTilerFactory(
reader=PGSTACBackend,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

no more need for custom MosaicTilerFactory (no more retry connection)

@attr.s
class MosaicSTACReader(MultiBaseReader):
"""Simplified STAC Reader.
class CustomSTACReader(pgstac_mosaic.CustomSTACReader):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can extent pgstac_mosaic.CustomSTACReader instead of creating a new custom one

@vincentsarago
Copy link
Contributor Author

Breaking changes, are described in developmentseed/titiler#396. TiTiler is trying to use better query parameters format, so moving away of , coma delimited list (bidx=1,2,3 -> bidx=1&bidx=2&bidx=3)

For the Tiler API here are the breaking changes:

  • resampling_method -> resampling
  • assets=b1,b2 -> assets=b1&assets=b2 (for MultiBaseTilerFactory)
  • new asset_bidx=image|1,2,3
  • new asset_expression=image|b1+b2

@vincentsarago vincentsarago marked this pull request as ready for review January 4, 2022 16:17
@mmcfarland
Copy link
Member

@vincentsarago Sorry I didn't see that this PR was waiting for us to approve a CI build. I just kicked that off, and you can see the build failure in the action logs.

pctiler/pctiler/reader.py:108: error: Name 'pgstac_mosaic' is not defined

@mmcfarland
Copy link
Member

Getting failures on preview endpoint:

tiler_1     |   File "/usr/local/lib/python3.8/site-packages/titiler/core/factory.py", line 745, in preview
tiler_1     |     data = src_dst.preview(
tiler_1     |   File "/usr/local/lib/python3.8/site-packages/rio_tiler/io/base.py", line 651, in preview
tiler_1     |     output = multi_arrays(assets, _reader, **kwargs)
tiler_1     |   File "/usr/local/lib/python3.8/site-packages/rio_tiler/tasks.py", line 71, in multi_arrays
tiler_1     |     [data for data, _ in filter_tasks(tasks, allowed_exceptions=allowed_exceptions)]
tiler_1     |   File "/usr/local/lib/python3.8/site-packages/rio_tiler/tasks.py", line 71, in <listcomp>
tiler_1     |     [data for data, _ in filter_tasks(tasks, allowed_exceptions=allowed_exceptions)]
tiler_1     |   File "/usr/local/lib/python3.8/site-packages/rio_tiler/tasks.py", line 34, in filter_tasks
tiler_1     |     yield future.result(), asset
tiler_1     |   File "/usr/local/lib/python3.8/concurrent/futures/_base.py", line 432, in result
tiler_1     |     return self.__get_result()
tiler_1     |   File "/usr/local/lib/python3.8/concurrent/futures/_base.py", line 388, in __get_result
tiler_1     |     raise self._exception
tiler_1     |   File "/usr/local/lib/python3.8/concurrent/futures/thread.py", line 57, in run
tiler_1     |     result = self.fn(*self.args, **self.kwargs)
tiler_1     |   File "/usr/local/lib/python3.8/site-packages/rio_tiler/io/base.py", line 643, in _reader
tiler_1     |     data = cog.preview(
tiler_1     |   File "/opt/src/pctiler/pctiler/reader_cog.py", line 227, in preview
tiler_1     |     if "goeseuwest.blob.core.windows.net" in self.filepath:
tiler_1     | AttributeError: 'CustomCOGReader' object has no attribute 'filepath'

Failure for mosaic/tile endpoint

tiler_1     |   File "/usr/local/lib/python3.8/site-packages/titiler/pgstac/factory.py", line 148, in tile
tiler_1     |     data, _ = src_dst.tile(
tiler_1     |   File "/opt/src/pctiler/pctiler/reader.py", line 194, in tile
tiler_1     |     mosaic_assets = self.assets_for_tile(
tiler_1     |   File "/opt/src/pctiler/pctiler/reader.py", line 132, in assets_for_tile
tiler_1     |     return self.get_assets(Polygon.from_bounds(*bbox), **kwargs)
tiler_1     |   File "/usr/local/lib/python3.8/site-packages/cachetools/decorators.py", line 17, in wrapper
tiler_1     |     k = key(*args, **kwargs)
tiler_1     |   File "/opt/src/pctiler/pctiler/reader.py", line 136, in <lambda>
tiler_1     |     key=lambda self, geom, **kwargs: hashkey(self.path, str(geom), **kwargs),
tiler_1     | AttributeError: 'PGSTACBackend' object has no attribute 'path'

/register endpoint seems to be working ok. Let me know if I can help debug these.

@mmcfarland
Copy link
Member

@vincentsarago things are generally working with the upgrade, though I've got a few test cases that are failing that I'm hoping are just related to a misunderstanding of the new TiTiler url params. Can you offer some suggestions here:

Previous working URL params

item/preview.png?collection=naip&items=fl_m_2708064_sw_17_060_20191215_20200113&assets=image&bidx=1,2,3

Not working

  • item/preview.png?collection=naip&item=fl_m_2708064_sw_17_060_20191215_20200113&assets=image&bidx=1&bidx=2&bidx=3
  • {"detail":"PNG driver doesn't support 5 bands. Must be 1 (grey), 2 (grey+alpha), 3 (rgb) or 4 (rgba) bands. "}

Also tried asset_bidx

  • item/preview.png?collection=naip&item=fl_m_2708064_sw_17_060_20191215_20200113&asset_bidx=image|1,2,3
  • {"detail":"assets must be defined either via expression or assets options."}

The tile version also fails with a different error

  • mosaic/tiles/c1e2f5aeaa27eaa1f6009ea21b0d60cc/WebMercatorQuad/12/969/1699@2x?assets=image&bidx=1&bidx=2&bidx=3&collection=naip&format=png
  • {"detail":"operands could not be broadcast together with shapes (4,512,512) (3,512,512) "}

I'll note that some NAIP / bidx request are succeeding, such as

  • item/preview.png?collection=naip&item=mi_m_4208544_sw_16_h_20160727&assets=image&bidx=1&bidx=2&bidx=3

so I don't think it's URL params, necessarily. The requests that fail to render on this branch succeed for the same item/asset/bands in our staging deployment. Something to do with the COGs themselves?

Looks like other collections with requests involving bands fail similarly

  • item/preview.png?collection=aster-l1t&item=AST_L1T_00308152006170202_20150515181209&assets=SWIR&bidx=1,3,5&nodata=0
  • {"detail":"PNG driver doesn't support 7 bands. Must be 1 (grey), 2 (grey+alpha), 3 (rgb) or 4 (rgba) bands. "}

All the other PC collections are rendering items correctly.

@vincentsarago
Copy link
Contributor Author

thanks for the feedback @mmcfarland

# before 
assets=image&bidx=1,2,3

# now
assets=image&asset_bidx=image|1,2,3

I realize now that rio-tiler/titiler could be smarter and maybe only accept asset_bidx=image|1,2,3

about the tile requests that succeed:

when passing bidx=1&bidx=2&bidx=3 this won't have any effects and thus fallback to the asset internal bands. It seems that you may have assets with 3 or 4 bands within the naip collection 🤷‍♂️

@mmcfarland
Copy link
Member

Ah, makes sense. I should have taken the error message at its word 😬.

I fixed up the asset_bidx and asset_expression usage and it's all good now, thanks for help.

Copy link
Member

@mmcfarland mmcfarland left a comment

Choose a reason for hiding this comment

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

Tested and working

@mmcfarland mmcfarland merged commit a10a7d5 into microsoft:main Jan 6, 2022
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.

3 participants