-
Notifications
You must be signed in to change notification settings - Fork 4
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
Save intermediate COG files to S3 #563
Conversation
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, assuming you get the tests working. Thanks for looking into this!
batch/scripts/cogify.sh
Outdated
|
||
ME=$(basename "$0") | ||
. get_arguments.sh "$@" | ||
#. get_arguments.sh "$@" |
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'm confused about why you commented this out. Doesn't this break the argument parsing? Did you just do this temporarily for some testing?
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.
Maybe this commented out line is the reason your tests are failing (in test_cog_asset
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.
oh yeah, good catch, that was to test running it locally
batch/scripts/cogify.sh
Outdated
if [ ! -f "cog.tif" ]; then | ||
gdal_translate merged.tif cog.tif -of COG -co COMPRESS=DEFLATE -co BLOCKSIZE="${BLOCK_SIZE}" -co BIGTIFF=IF_SAFER -co NUM_THREADS=ALL_CPUS --config GDAL_CACHEMAX 70% --config GDAL_NUM_THREADS ALL_CPUS | ||
fi | ||
gdal_translate merged.tif cog.tif -of COG -co COMPRESS=DEFLATE -co BLOCKSIZE="${BLOCK_SIZE}" -co BIGTIFF=IF_SAFER -co NUM_THREADS=ALL_CPUS -co OVERVIEWS=FORCE_USE_EXISTING --config GDAL_CACHEMAX 70% --config GDAL_NUM_THREADS ALL_CPUS |
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. Just wondering - so I assume that the OVERVIEWS=FORCE_USE_EXISTING is the option that makes sure that merged.tif.ovr is used, even though it is not explicitly named on the command line?
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.
Exactly, that confused me too, but there's no option to specify an overview file, you just need to follow the naming convention.
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #563 +/- ##
========================================
Coverage 81.92% 81.92%
========================================
Files 126 126
Lines 5692 5693 +1
========================================
+ Hits 4663 4664 +1
Misses 1029 1029
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Nice!
Pull request checklist
Please check if your PR fulfills the following requirements:
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Save intermediate files for COGs locally, with the incorrect expectations the /tmp folder persists between spot instance retries.
Issue Number: GTC-2908
What is the new behavior?
-ro
option so we can also save them to S3, and only save them once they completeDoes this introduce a breaking change?
Other information