-
Notifications
You must be signed in to change notification settings - Fork 2
Conversation
@@ -95,8 +95,4 @@ def handle_message(metadata_key: str) -> None: | |||
if root_catalog.get_child(dataset_metadata.id) is None: | |||
root_catalog.add_child(child=dataset_metadata, strategy=GeostoreSTACLayoutStrategy()) | |||
|
|||
root_catalog.normalize_hrefs( |
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.
We do want to normalize hrefs across the Geostore (hopefully we have a test to enforce that). Have you checked that we have code elswehere for that and/or a test for this?
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.
We definitely don't need to normalize every href every time we add a dataset though, just need to do the catalog.json and all the STAC files in the child / dataset that is being added. Can ignore all the sibling dataset stac files.
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 was addressed in stac-utils/pystac#294 (if I read the PR correctly). Nevertheless, tests are failing which means more work is needed to figure out what is needed to fix this issue.
Changing PR to draft since tests are failing. |
0300e6e
to
82b591b
Compare
Looking through the test failures:
In summary, to make this work ASAP we could:
|
09fb141
to
3b3a279
Compare
tests/test_populate_catalog.py
Outdated
assert catalog_json[STAC_LINKS_KEY] == expected_root_catalog_links | ||
updated_catalog_json = [] | ||
for i in catalog_json[STAC_LINKS_KEY]: | ||
del i["title"] |
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.
Rather than deleting the title from the actual state you could simply remove the non-existent title from the expected state. That should result in less code overall, and very slightly more coverage.
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.
Do you mean something like this bb4fd62?
It was my initial attempt, but it didn't work. Maybe I have missed something?
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.
Something like bb4fd62, except some of the titles should still be there.
root_catalog.normalize_hrefs( | ||
f"{S3_URL_PREFIX}{Resource.STORAGE_BUCKET_NAME.resource_name}", | ||
strategy=GeostoreSTACLayoutStrategy(), | ||
) |
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.
Isn't there some missing code to actually replace the root href
?
> assert item_json[STAC_LINKS_KEY] == expected_item_links
E AssertionError: assert [{'href': './CCRrWAn9FPPOyNFd1Q6I.json',\n 'rel': 'root',\n 'type': 'application/json'},\n {'href': './CCRrWAn9FPPOyNFd1Q6I.json',\n 'rel': 'parent',\n 'type': 'application/json'}] == [{'href': '../catalog.json', 'rel': 'root', 'type': 'application/json'},\n {'href': './CCRrWAn9FPPOyNFd1Q6I.json',\n 'rel': 'parent',\n 'type': 'application/json'}]
E At index 0 diff: {'rel': 'root', 'href': './CCRrWAn9FPPOyNFd1Q6I.json', 'type': 'application/json'} != {'rel': 'root', 'href': '../catalog.json', 'type': 'application/json'}
E Full diff:
E [
E - {'href': '../catalog.json',
E + {'href': './CCRrWAn9FPPOyNFd1Q6I.json',
E 'rel': 'root',
E 'type': 'application/json'},
E {'href': './CCRrWAn9FPPOyNFd1Q6I.json',
E 'rel': 'parent',
E 'type': 'application/json'},
E ]
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.
Took a while for me to trace this down. There are two options to tackle this bug:
root_catalog.save_object()
or root_catalog.save(catalog_type=CatalogType.SELF_CONTAINED)
see populate_catalog/task.py
The first option save_object
is the workaround suggested by Pystac (see stac-utils/pystac#284). This would be the ideal implementation as none of the metadata gets duplicated during dataset version creation (at least in in my own tests); however, from what I can tell, this options eludes importer
from being called in import_metadata_file/task.py
(which is why you are seeing the error message above - something I tried in the early commit). Why this may be the case exactly, I am still slightly unclear. I have tried tracing this down but with no success thus far. Perhaps you might have a better understanding of how Geostore utilizes Pystac to shine some light on why this may be? I will continue to explore and will update if I am able to work this out.
The second option root_catalog.save(catalog_type=CatalogType.SELF_CONTAINED)
is the option I went with in this PR. This appears to only be a partial fix. It prevents item.json
from being updated with every new dataset version creation; however, it doesn't prevent collection.json
from being updated every time a new dataset is added or updated.
9b50390
to
26b008f
Compare
26b008f
to
02bc373
Compare
@@ -16,6 +16,7 @@ | |||
from ..types import JsonObject | |||
|
|||
S3_BODY_KEY = "Body" | |||
CATALOG_FILENAME = "catalog.json" |
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 constant already exists in geostore/populate_catalog/task.py
, so it should probably be moved to a file in the geostore
directory (so that it's shared between subdirectories at deployment time) and imported.
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.
Have changed this to reference geostore/populate_catalog/task.py
(for now). Good idea to move this to another file, but a quick look shows it is reference in quite a few places, so I might move this change to another PR.
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.
Spoke too soon. The value needs to be accessible via the lambda function. Changing this back to ../catalog.json
for now and will create another PR to address the above.
9340732
to
a86b121
Compare
Pystac normalize_hrefs resolves all links and mutates the entire tree, causing all .json metadata to update. Removing normalize_hrefs fixes this particular issue. GeostoreSTACStrategy get_item_href is not called post removal; however, this may be relevant again in the future as the codebase grows. Excluding this function from coverage, for now.
1ceaf00
to
3a98ed8
Compare
Missing / wrong root link is causing tests to fail post removal of normalize_href. This commit modifies the root URL as it's currently specified in the tests.
3a98ed8
to
e60ac15
Compare
e60ac15
to
b67704d
Compare
Test values need to match expected values, post removal of pystac normalize_href
Removing normalized_href requires root href key to be updated to "../catalog.json". update_root_link function was added to "geostore/import_metadata_file/task.py". To ensure full coverage, this test is added to ensure both root and non-root href_key are covered.
b67704d
to
7557080
Compare
The get_item_href function isn't called post normalized_href removal; however, it is needed to override all the abstract methods in the parent class before it can be instantiated. raise NotImplementedError is added to ensure pragma: no cover is removed if this method is called again in the future as the codebase expand.
187c6d1
to
d122a5c
Compare
Pystac
normalize_hrefs
resolves all links and mutates the entire tree, causing<item>.json
andcollection.json
for all datasets within the catalog to update.save_object
only saves the particular entity within the catalog, preventing unnecessary metadata duplication.Reference
stac-utils/pystac#284
stac-utils/pystac#294
stac-utils/pystac#90