materialsproject / atomate2

atomate2 is a library of computational materials science workflows
https://materialsproject.github.io/atomate2/
Other
167 stars 96 forks source link

Fix MPID assignment in electrode workflow #956

Closed esoteric-ephemera closed 2 months ago

esoteric-ephemera commented 3 months ago

Minor bugfix: in atomate2.common.jobs.electrode, the utility functions get_computed_entries, get_structure_group_doc, and get_insertion_electrode_doc assign MPIDs via UUIDs, which leads to pydantic errors when the documents are serialized (an MPID has to have the form mp-<int>).

Thanks to @RobertaPascazio for catching this. Copy @jmmshn to make sure this is still compliant with the flow format.

jmmshn commented 3 months ago

So this came out of a need to peg the id of the output document to the "lowest available ID". In jobflow the issue of sortable ids was discussed here: https://github.com/materialsproject/jobflow/issues/519

This is to help prevent id drift over time. Since jobflow has no concept of an mpid but we'd like to recycle the insertion electrode code this seemed like a reasonable compromise. Since we have a situation where many document models are used in systems where a formal MPID is not required, I'm wondering if it makes sense to make the MPID definition a bit broader.

esoteric-ephemera commented 3 months ago

Thanks, @jmmshn. It might be good in the long run to make emmet's MPID class more broad in scope (like accepting alphanumeric str input), but that would require a bigger discussion on the MP side.

For right now, the flow can't run successfully without passing UID_TYPE="ulid" to JobStore. Since this isn't documented in the flow (I only saw this in the electrode vasp tests), I'm changing this so that ULIDs are assigned if an entry_id can't be parsed as an emmet MPID

utf commented 2 months ago

Thanks @esoteric-ephemera