bring in code from maggma.api to emmet.api #1267
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1267 +/- ##
==========================================
- Coverage 88.92% 88.81% -0.11%
==========================================
Files 161 179 +18
Lines 16545 17461 +916
==========================================
+ Hits 14712 15508 +796
- Misses 1833 1953 +120 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Thanks @kbuma! I removed the maggma[api] dependency in setup.py, reran the dependency upgrades, and merged it into the branch for this PR. That way the tests in this PR run with the updated dependencies. Looks like there are some maggma imports left, and some dependencies from maggma missing (e.g. uvicorn). Could you update the setup.py accordingly? I can regenerate the dependency files for testing afterwards. Thanks!
|
@tschaume should be ready for final review |
There was a problem hiding this comment.
Looks good! Left some minor comments for you to consider.
Just to confirm, this PR shouldn't break the API, right? Does it depend on a new emmet-core release? Ping me when you had the chance to look through my suggestions, and I'll try to bring up the API in the docker-compose stack to make sure it still works with web. Thanks!
|
|
||
| @field_validator("errors", mode="before") | ||
| @classmethod | ||
| def check_consistency(cls, v, values: ValidationInfo): |
| def default_meta(cls, v: Any, values: ValidationInfo): | ||
| if v is None or hasattr(v, "model_dump"): | ||
| v = Meta().model_dump() # type: ignore[call-arg] | ||
| if v.get("total_doc", None) is None: |
There was a problem hiding this comment.
Second arg None to get isn't needed I think. get returns None by default.
| if getattr(values, "data", None) is not None: | ||
| v["total_doc"] = len(values.data) | ||
| else: | ||
| v["total_doc"] = 0 |
There was a problem hiding this comment.
Could be shortened to inline v["total_doc"] = ... if ... else ...
| remainder = { | ||
| k: v | ||
| for query in queries | ||
| for k, v in query.items() | ||
| if k not in ["criteria", "properties"] | ||
| } |
There was a problem hiding this comment.
Could be removed if using sub_query.pop() in preceding for loop and if it's ok to modify queries.
| remainder = { | ||
| k: v | ||
| for query in queries | ||
| for k, v in query.items() | ||
| if k not in ["criteria", "properties", "facets"] | ||
| } |
| def api_sanitize( | ||
| pydantic_model: BaseModel, | ||
| fields_to_leave: str | None = None, | ||
| allow_dict_msonable=False, |
There was a problem hiding this comment.
allow_dict_msonable: bool = False
| if field_type is not None and allow_dict_msonable: | ||
| if lenient_issubclass(field_type, MSONable): | ||
| field_type = allow_msonable_dict(field_type) |
There was a problem hiding this comment.
The names of arg ('allow_dict_msonable) vs function (allow_msonable_dict`) tripped me up for a second here :) Maybe it can be changed to be more distinguishable.
| if lenient_issubclass(field_type, MSONable): | ||
| field_type = allow_msonable_dict(field_type) | ||
| else: | ||
| for sub_type in get_args(field_type): |
There was a problem hiding this comment.
We're only going down two levels looking for msonable dicts?
| if v.get("@class", "") != monty_cls.__name__: | ||
| errors.append("@class") | ||
|
|
||
| if len(errors) > 0: |
There was a problem hiding this comment.
if errors should suffice here, right?
| def get_flat_models_from_model( | ||
| model: BaseModel, known_models: set[BaseModel] = set() | ||
| ) -> set[BaseModel]: | ||
| """Get all sub-models from a pydantic model. | ||
|
|
||
| Args: | ||
| model (BaseModel): Pydantic model | ||
| known_models (set[BaseModel]) : set of identified pydantic sub-models | ||
|
|
||
| Returns: | ||
| (set[BaseModel]): Set of pydantic models | ||
| """ | ||
| known_models = set() | ||
|
|
||
| def get_sub_models(model: Any): | ||
| if lenient_issubclass(model, BaseModel): | ||
| known_models.add(model) | ||
| for field_info in model.model_fields.values(): | ||
| get_sub_models(field_info.annotation) | ||
| else: | ||
| for type_anno in get_args(model): | ||
| get_sub_models(type_anno) | ||
|
|
||
| get_sub_models(model) | ||
| return known_models |
There was a problem hiding this comment.
Does pydantic have a built-in function for this?
|
@tschaume We purposefully did not try to improve the code of the files moved over from maggma in order to reduce risk. You'll see we've tried not to modify the contents when possible. Let us know if you'd like us to do that. Otherwise, we will forego addressing the suggestions as part of this PR and address when we do the next set of changes related to removing the Stores. |
The code in maggma.api is only used by emmet. Bringing that code in here so that it does not need to be maintained as part of maggma.
Contributor Checklist