-
Notifications
You must be signed in to change notification settings - Fork 2
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
GTC-2580 Create COG assets of integrated alerts in datapump #145
Conversation
Merge private subnet changes up
Added extra cog_asset_parameters argument to RasterVersionUpdateJob(). Updated IntegratedAlertsSync() to create a new "intensity" raster with every value set to 255, and then used cog_asset_parameters to create two new COGs corresponding to the date_conf and intensity rasters.
And remove some code that won't be needed for testing on staging.
Removes Localstack-Pro dependency.
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.
Looks good overall, one small change request and one optional change request at your discretion
src/datapump/jobs/version_update.py
Outdated
assets = client.get_assets(self.dataset, self.version) | ||
asset_id = "" | ||
for asset in assets: | ||
if asset["asset_type"] == "Raster tile set" and f"/{co.source_pixel_meaning}/" in asset["asset_uri"]: |
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.
Why don't you just check the pixel meaning on the asset instead of looking for it in the URI? I think it should be on there, or you might need to get the creation options. I think might be a little neater and less prone to error if two pixel meanings have overlapping names (e.g. date
and date_conf
)
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.
Thanks for the review! I was just trying to avoid one extra Data API access. Added a new simple function to access the creation_options
of an asset, and change the test to compare directly with the pixel_meaning
of the asset.
|
||
class CogAssetParameters(StrictBaseModel): | ||
implementation: str | ||
source_pixel_meaning: str |
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.
This works, I think my preference would be to actually link it to tile set job and get the ID from there, but that might make it fairly complicated
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.
I think I'll leave things as they are. Since pixel_meaning
is unique, this seems to be the simplest way to link up the jobs, especially given one of the tile set jobs is a tile_set_parameters job, and the other is an aux_tile_set_parameters job.
When linking COG job up to previous raster tile set, get the pixel meaning of an asset from the creation_options, rather than just looking at the URI. Also, don't break out of loop, so we can look for duplicate pixel_meaning matches (not very possible, I think).
GTC-2580 Create COG assets of integrated alerts in datapump
Added extra cog_asset_parameters argument to RasterVersionUpdateJob(),
which specifies any COG jobs to run after any aux_asset tile jobs.
Updated IntegratedAlertsSync() to create a new "intensity" raster with
every value set to 255, and then used cog_asset_parameters to create two
new COGs corresponding to the date_conf and intensity rasters.
I have a few changes (forcing creation of a new integrated_alerts version,
disabling Geotrellis job) which I added for testing in staging, but will remove
in develop before doing the merge to master for production.