Skip to content
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

add an option to upload the open data #247

Merged
merged 27 commits into from
Mar 20, 2024

Conversation

cirrusasf
Copy link
Contributor

No description provided.

@cirrusasf cirrusasf added the patch Bump the patch version number of this project label Mar 19, 2024
@cirrusasf cirrusasf marked this pull request as ready for review March 19, 2024 00:49
@cirrusasf cirrusasf requested a review from a team March 19, 2024 00:49
Copy link
Contributor

@jhkennedy jhkennedy left a comment

Choose a reason for hiding this comment

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

Off to a good start!

return var.latitude, var.longitude


def point_to_prefix(dir_path: str, lat: float, lon: float) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're hard-coding the prefix anyways, I think it'd be better to take platform as input and instead return the entire prefix from this function so that we'd end up calling it like:

>>> platform = get_platform(scene)
>>> lat, lon = get_lon_lat_from_ncfile(file)
>>> point_to_prefix(platform, lat, lon)
'velocity_image_pair/landsatOLI/v02/S80W100'

Copy link
Contributor

Choose a reason for hiding this comment

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

done!

return dirstring


def get_opendata_prefix(file: Path, scene: str):
Copy link
Contributor

Choose a reason for hiding this comment

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

you can also get the scene name from the file name

scene = file.name.split('_X_')[0]

which is better since we sort the granules on input. Either that, or when this is called, you should pass it g1:
https://github.com/ASFHyP3/hyp3-autorift/blob/develop/src/hyp3_autorift/process.py#L538

Copy link
Contributor

Choose a reason for hiding this comment

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

done!

src/hyp3_autorift/process.py Outdated Show resolved Hide resolved
src/hyp3_autorift/process.py Outdated Show resolved Hide resolved
src/hyp3_autorift/process.py Outdated Show resolved Hide resolved
src/hyp3_autorift/process.py Outdated Show resolved Hide resolved
src/hyp3_autorift/process.py Outdated Show resolved Hide resolved
@forrestfwilliams
Copy link
Contributor

OK @cirrusasf and @jhkennedy I believe I've addressed all the notes from the code review. Next thing we should do is add tests for point_to_prefix and get_opendata_prefix to test_process.py.

Comment on lines 597 to 601
if args.publish:
prefix = get_opendata_prefix(product_file)
upload_file_to_s3(product_file, OPEN_DATA_BUCKET, prefix)
upload_file_to_s3(browse_file, OPEN_DATA_BUCKET, prefix)
upload_file_to_s3(thumbnail_file, OPEN_DATA_BUCKET, prefix)
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking about it, I think I'd rather do that after we upload to the HyP3 content bucket -- that way, if this part breaks, we still have a copy in HyP3 and can copy the files over without having to reprocess.

What do you thinK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

very good idea. I will change the sequence.

@@ -538,6 +592,13 @@ def main():
g1, g2 = sorted(args.granules, key=get_datetime)

product_file, browse_file = process(g1, g2, parameter_file=args.parameter_file, naming_scheme=args.naming_scheme)
thumbnail_file = create_thumbnail(browse_file)
Copy link
Contributor

Choose a reason for hiding this comment

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

moving this here means we can remove the duplicate line 606

Comment on lines 366 to 369
lat_lon_prefix_component = point_to_prefix(platform_shortname, lat, lon)

dir_path = f'velocity_image_pair/{PLATFORM_SHORTNAME_LONGNAME_MAPPING[platform_shortname]}/v02'
prefix = os.path.join(dir_path, lat_lon_prefix_component)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd still prefer all of this logic in point_to_prefix so it's actually returning the prefix. Otherwise we should change the function name.

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@forrestfwilliams forrestfwilliams left a comment

Choose a reason for hiding this comment

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

@cirrusasf a couple small suggestions, but I'll be ready to approve after this!

CHANGELOG.md Outdated
@@ -6,6 +6,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [PEP 440](https://www.python.org/dev/peps/pep-0440/)
and uses [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [0.14.2]
### Added
* `--publish` option has been added to the HyP3 entry point to publish product to the ITS_LIVE AWS Open Data bucket, `s3://its-live-data`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* `--publish` option has been added to the HyP3 entry point to publish product to the ITS_LIVE AWS Open Data bucket, `s3://its-live-data`.
* `--publish` option has been added to the HyP3 entry point to publish product to the ITS_LIVE AWS Open Data bucket, `s3://its-live-data`.
* `upload_file_to_s3_with_upload_access_keys` to perform S3 uploads with credentialed S3 clients.
* use of `UPLOAD_ACCESS_KEY_ID` and `UPLOAD_ACCESS_KEY_SECRET` to upload products to write-protected bucket.

Co-authored-by: Forrest Williams <31411324+forrestfwilliams@users.noreply.github.com>
@cirrusasf cirrusasf merged commit ecdb419 into develop Mar 20, 2024
7 checks passed
@cirrusasf cirrusasf deleted the auto_upload_its_live_data_bucket branch March 20, 2024 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Bump the patch version number of this project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants