Parse dataset id from downloaded nml #1256
Conversation
There was a problem hiding this comment.
Code changes look ok.
Perhaps add a unit test to prevent this from happening again. Maybe something like:
test_dataset_id = "http://localhost:9000/datasets/<some existing dataset>"
test_annotation_id = "http://localhost:9000/annotations/<some existing annotation>/#128,128,128,0,1.3"
with wk.webknossos_context(token="<token>", url="http://localhost:9000"):
dataset = wk.Dataset.open_remote(test_dataset_id)
dataset_from_annotation = wk.Annotation.open_as_remote_dataset(test_annotation_id)
assert dataset.id == dataset_from_annotation.id
Sadly its not that easy. A dataset does not have an I tried to add a test now that creates its own dataset, uploads and downloads an annotation, renames the DS and then tries to access the dataset from the re-downloaded annotation. Hopefully, I can get this working in the Ci 🥴 |
- improve existing test to check whether the dataset id is parsed from a zipped annotation
There was a problem hiding this comment.
I added the datasetId property to the nml to check against that in the test test_annotation_from_wkw_zip_file test
webknossos/tests/test_annotation.py
Outdated
|
|
||
|
|
||
| # Regression test for: https://github.com/scalableminds/webknossos-libs/pull/1256 | ||
| @pytest.mark.use_proxay | ||
| def test_dataset_access_via_annotation(tmp_path: Path) -> None: | ||
| url = "http://localhost:9000/datasets/Organization_X/l4_sample" | ||
| token = os.getenv("WK_TOKEN") | ||
| with wk.webknossos_context("http://localhost:9000", token): | ||
| dataset_to_reupload = wk.Dataset.download( | ||
| url, path=Path(tmp_path) / "sample_ds", bbox=wk.BoundingBox((3164, 3212, 1017), (10, 10, 10)) | ||
| ) | ||
| renameable_dataset = dataset_to_reupload.upload("name_to_replace") | ||
| path = TESTDATA_DIR / "annotations" / "l4_sample__explorational__suser__94b271.zip" | ||
| annotation_from_file = wk.Annotation.load(path) | ||
| annotation_from_file.organization_id = "Organization_X" | ||
| annotation_from_file.dataset_name = renameable_dataset.name | ||
| annotation_from_file.dataset_id = renameable_dataset._dataset_id | ||
| test_token = os.getenv("WK_TOKEN") | ||
| with wk.webknossos_context("http://localhost:9000", test_token): | ||
| url = annotation_from_file.upload() | ||
| annotation = wk.Annotation.download(url) | ||
| assert annotation.dataset_name == "name_to_replace" | ||
| assert len(list(annotation.skeleton.flattened_trees())) == 1 | ||
|
|
||
| renameable_dataset.name = "replaced_name" | ||
| # Test whether the DS can still be accessed via the annotation after the renaming. | ||
| dataset_from_annotation = annotation.get_remote_annotation_dataset() | ||
| assert dataset_from_annotation._dataset_id == renameable_dataset._dataset_id |
There was a problem hiding this comment.
As I am currently struggling to get this test working. I removed this test so we can add it in a follow up pr to not block merging the fix
There was a problem hiding this comment.
see #1258
Description:
Prior to these changes, wklibs missed to parse the datasetId provided in an nml of an annotation. This leads to issue / the libs crashing when parsing annotations from a renamed dataset.
To reproduce and confirm the fix run something like with an annotation of a renamed dataset.
On the master this code should crash but not on this branch.
Issues:
Todos:
Make sure to delete unnecessary points or to check all before merging: