Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix MPID assignment in electrode workflow #956

Merged
merged 8 commits into from
Aug 20, 2024

Conversation

esoteric-ephemera
Copy link
Contributor

@esoteric-ephemera esoteric-ephemera commented Aug 15, 2024

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
Copy link
Contributor

jmmshn commented Aug 15, 2024

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: materialsproject/jobflow#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
Copy link
Contributor Author

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
Copy link
Member

utf commented Aug 20, 2024

Thanks @esoteric-ephemera

@utf utf merged commit 7c74022 into materialsproject:main Aug 20, 2024
6 checks passed
@utf utf added the fix Bug fix PR label Aug 20, 2024
@esoteric-ephemera esoteric-ephemera deleted the fix_mpid_electrode branch August 20, 2024 15:41
hrushikesh-s pushed a commit to hrushikesh-s/atomate2 that referenced this pull request Nov 16, 2024
* fix mpid in electrode

* fix mpid in electrode

* ensure unique mpid

* precommit

* change mpid assignment logic to assign ULIDs on the fly as needed

* precommit

* remove missing test fixture call
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants