Enable metadata access for remote datasets. #1163
Conversation
…sos-libs into accessing-metadata
| @@ -0,0 +1,32 @@ | |||
| import webknossos as wk | |||
| from webknossos.client.api_client.models import ApiMetadata | |||
There was a problem hiding this comment.
The api_client.models classes should stay private to our own code, to enable future changes to the HTTP API without changing the user-exposed python API.
Please create an abstraction layer between ApiMetadata and the user-exposed metadata in Dataset and Folder classes. Maybe even without a type property, but using python types instead? The type property could be inferred in the conversion.
Maybe it could even be just a dict? Not sure about that, maybe @philippotto or @MichaelBuessemeyer have a preference.
There was a problem hiding this comment.
I think, a dict mapping from key to primitive would be cool, but I'm not sure it's worth the complexity. in the end, this api should be easy and clear to use. directly using primitives might make people believe that they can also put python objects there. so, it would definitely need thorough validation.
I don't have a strong opinion about this, though.
There was a problem hiding this comment.
I decided against a dict so far because it seems that the key is not unique so far and as philipp already mentioned the value types must be validated somehow to allow conversion to TypeScript types.
There was a problem hiding this comment.
because it seems that the key is not unique so far
to which implementation are you referring? the wklibs one? because in wk, it should be unique (if not, we should fix it 😅)
There was a problem hiding this comment.
The wk one, it seems that the creation of duplicates is restricted by the frontend but not in the backend. I can send the same key-value pair multiple times with the API without an error. I just tested it with the l4_test_mark dataset.
There was a problem hiding this comment.
Sorry for entering the thread so late
because in wk, it should be unique (if not, we should fix it 😅)
The frontend does the validation that no key is duplicated. The backend just takes what the frontend sends and saves & serves it.
Do you think a validation would be helpful? I can certainly build that :)
There was a problem hiding this comment.
Do you think a validation would be helpful?
I changed the wk-libs implementation to use a dict now. So when using the libs as intended, the key should be unique too. Nevertheless, a validation would be helpful to ensure that the API is used correct.
There was a problem hiding this comment.
Do you think a validation would be helpful? I can certainly build that :)
We should either fully support duplicates, or ensure that none arise. So with the current design, I’d say yes, the backend should assert this in the update routes. Could you create an issue for that? It’s not super urgent, though.
There was a problem hiding this comment.
I created an issue for this: scalableminds/webknossos#8068
And as I wanted a rather quick and simple task right now, I simply started the implementation 🙈
There was a problem hiding this comment.
Looking good :)
I tested and the access in the style of the example worked for me. However, intuitively, I first tried dataset.metadata["test"] = "asdf", which did not error, but also did have any effect. Do you think it would be simple to support this case as well? (Or alternatively somehow freeze this object, so that an error is provided, suggesting the other access style?) If this is very hard to achieve, then this current version is also ok.
As it was currently just a dict neither freezing nor overwriting |
|
Interesting idea! I like this access pattern from the user perspective. However, I don’t know enough about python and its conventions to fully know what unwanted side-effects this specialized dict may have 🤔 Also I’m not certain I can review its implementation. Maybe we can get a second opinion? Maybe @daniel-wer could also have a look? |
| _T = TypeVar("_T", bound="Metadata") | ||
|
|
||
|
|
||
| class Metadata(dict): |
There was a problem hiding this comment.
Have you considered implementing a MutableMapping directly? I think that would remove some trouble with the persistence that a dict does automatically.
| class Metadata(dict): | |
| class Metadata(MutableMapping): |
Description:
Issues:
Todos:
Make sure to delete unnecessary points or to check all before merging:
Updated Changelog
Considered adding this to the Examples
blocked by Folder And Dataset Metadata webknossos#7886
How to test:
Just run the example in
accessing_metadata.pyand enter your webknossos token when asked. After execution, the datasetl4_sampleand the folderDatasets/Demo-Datasetsshould have an entry with metadata.