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

TaskDoc refactor #971

Merged
merged 31 commits into from
May 21, 2024
Merged

TaskDoc refactor #971

merged 31 commits into from
May 21, 2024

Conversation

esoteric-ephemera
Copy link
Collaborator

@esoteric-ephemera esoteric-ephemera commented Mar 20, 2024

Summary

The main goal is removing atomate dependencies from emmet, and transitioning from using emmet.core.vasp.task_valid.TaskDocumement to emmet.core.tasks.TaskDoc. This will allow for eventual deprecation / aliasing of TaskDocument

Completed:

Changes to TaskDoc and its fields are required since it is not directly compatible with TaskDocument:

  • The fields of TaskDocument are deserialized, thus TaskDocument.calcs_reversed[:].input will be a dict whereas TaskDocument.calcs_reversed[:].input is a CalculationInput object. Calling .model_dump() doesn't help since this isn't recursive (e.g., the structure in CalculationInput will still be serialized)
  • Added a BaseCalculationModel to emmet.core.calculation which just wraps the pydantic BaseModel with a .get method to emulate dict key access (needed in the ValidationDoc)
  • Added a few fields to add missing attrs from TaskDocument: calc_type, run_type, and a post init update for task_type
  • Because the TaskDoc accepts extra model input, and the way that pydantic looks for non-field attrs in the class, there are new private fields defined on TaskDoc. pydantic first looks for attrs in the model extra field, but then raises an AttributeError if it can't find it, without looking to see if hasattr(class,attr). There's a longer explanation in a comment near _calc_type

Will be part of a separate PR:
## To Do:
- Remove atomate dependence from emmet-cli: VaspCalcDb --> generic maggma store
- Improve credential handling in emmet-cli settings

@esoteric-ephemera esoteric-ephemera marked this pull request as draft March 20, 2024 19:25
@codecov-commenter
Copy link

codecov-commenter commented Mar 20, 2024

Codecov Report

Attention: Patch coverage is 11.76471% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 77.11%. Comparing base (d9d7d21) to head (7ab775f).

Files Patch % Lines
emmet-core/emmet/core/vasp/validation.py 6.66% 14 Missing ⚠️
emmet-core/emmet/core/utils.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #971      +/-   ##
==========================================
- Coverage   77.24%   77.11%   -0.14%     
==========================================
  Files          75       75              
  Lines        4329     4339      +10     
==========================================
+ Hits         3344     3346       +2     
- Misses        985      993       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mkhorton
Copy link
Member

If there are breaking changes to TaskDoc, eg for current users of atomate2, could a migration script be provided?

@esoteric-ephemera
Copy link
Collaborator Author

Hi @mkhorton, there shouldn't be breaking changes to TaskDoc, I've just had to add fields/methods to it for improved compatibility with both atomate2 and TaskDocument

A good example is the TaskDoc.task_type field, which actually corresponds to TaskDocument.calc_type in the atomate2 schema. The calc_type field is the union of TaskDocument.task_type and TaskDocument.run_type (a CalcType)

For compatibility with atomate2, TaskDoc.task_type still accepts CalcType as input, but for backwards compatibility with TaskDocument, there are new calc_type and run_type fields on TaskDoc

I presume that the TaskDoc.task_type field was intended to reduce some of the redundancy of these three fields, but some glue code was needed for full backwards compatibility. These added fields won't be used in atomate2

Apologies in advance for the long-winded answer

@esoteric-ephemera esoteric-ephemera changed the title [WIP] TaskDoc refactor TaskDoc refactor May 21, 2024
@esoteric-ephemera esoteric-ephemera marked this pull request as ready for review May 21, 2024 22:18
@tsmathis
Copy link
Collaborator

merging as rc

@tsmathis tsmathis merged commit ec4d0b5 into materialsproject:main May 21, 2024
10 checks passed
@esoteric-ephemera esoteric-ephemera deleted the taskdoc branch May 30, 2024 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants