Fix issue with uploading - #121
Conversation
| @field_validator("ids_list", mode="before") | ||
| @classmethod | ||
| def _coerce_ids_list(cls, v: Any) -> Any: | ||
| """Accept the display-string form of the IDS list, ``"[a, b, c]"``.""" | ||
| if not isinstance(v, str): | ||
| return v | ||
| text = v.strip() | ||
| if text.startswith("[") and text.endswith("]"): | ||
| text = text[1:-1] | ||
| return [name.strip() for name in text.split(",") if name.strip()] | ||
|
|
There was a problem hiding this comment.
Correct me if I'm wrong, but this shouldn't be necessary.
Pydantic should have already excellent support for deserializing a string ('[disruption, equilibrium..rofiles, summary, wall]') to list of strings (or other Any objects)
Is it possible that somehow the string got quoted doubly?
There was a problem hiding this comment.
I don't think it's a double-quoting issue. The string doesn't seem to be JSON to begin with.
[a, b, c] is not valid json. ["a", "b", "c"] is
It's built by hand as a display string in database/models/simulation.py:224:
self.set_meta("input_ids", "[{}]".format(", ".join(all_input_idss)))
There was a problem hiding this comment.
Let's fix that anomaly at the source then, no?
Thanks for showing where that string came from, btw!
There was a problem hiding this comment.
Fixing at the source would also have my preference, but that would mean that older clients will not work anymore.
(and we'd also have to write migrations for the database, because this string is stored there :(
There was a problem hiding this comment.
Bummer!
Maybe in a future major release, all the serialization at the source could be replaced by Pydantic's serialization, and then any "serialization by hand" should be forbidden?
There was a problem hiding this comment.
I don't think it's a double-quoting issue. The string doesn't seem to be JSON to begin with.
[a, b, c]is not valid json.["a", "b", "c"]isIt's built by hand as a display string in database/models/simulation.py:224:
self.set_meta("input_ids", "[{}]".format(", ".join(all_input_idss)))
[...]
Fixing at the source would also have my preference, but that would mean that older clients will not work anymore.(and we'd also have to write migrations for the database, because this string is stored there :(
This appears to be a bug in current clients, because the database model expects JSON already:
SimDB/src/simdb/database/models/simulation.py
Lines 123 to 129 in 948d61a
Addressing this would likely obsolete src/simdb/database/models/utils.py in favor of a few of Pydantic's serialization and validation methods.
There was a problem hiding this comment.
Fixing at the source would also have my preference, but that would mean that older clients will not work anymore.
(and we'd also have to write migrations for the database, because this string is stored there :(
From #120 older clients are not going to work anymore anyway or am I missing something?
In practice this next release from develop will be most probably 1.0, so a major bump.
Plus we already require database migration with Alembic, so it might be the right time to also fix this serialization in your preferred way now no?
3 participants
The IDS list got deserialized wrongly. Also fixes an issue with calculating the checksum of IMAS data in a docker setup