New REQUIRED level properties #153

CasperWA · 2020-02-03T12:51:42Z

Fixes #114
Fixes #154

See commit message.

Note:

Missing tests for this update.
Also, I should update the "documentation" of the various fields according to the changes introduced in Common rules for inclusion and querying OPTIMADE#210

ltalirz

@CasperWA I have a few minor comments,
but I also need to say that I didn't quite get the main point of the PR, so probably better if we have a quick chat tomorrow (or someone else could review)

optimade/models/entries.py

ltalirz · 2020-02-03T19:19:08Z

optimade/models/structures.py

@@ -84,7 +84,7 @@ def validate_chemical_symbols(cls, v):
    def validate_concentration(cls, v, values):
        if len(v) != len(values.get("chemical_symbols", [])):
            raise ValueError(
-                f"Length of concentration ({len(v)}) MUST equal length of chemical_symbols ({len(values.get('chemical_symbols', []))})"
+                f"Length of concentration ({len(v)}) MUST equal length of chemical_symbols ({len(values.get('chemical_symbols', 'Not specified'))})"


Is 'Not specified' better than None here?

I guess it's more informative if you're developing a server, which these messages are mainly for.

Edit: None might be a deliberately set value.

CasperWA · 2020-02-04T18:47:22Z

I have added a test, repeated 3 times for each endpoint "links", "references" and "structures".
It sets some values for a response_fields request, retrieves all top-level fields, as well as all attributes fields, and then compares to what would be expected according to the new rules, and with what we have in our "database".

CasperWA · 2020-02-04T18:49:21Z

Furthermore, I have opened an issue Materials-Consortia/OPTIMADE#250 to clarify the spec description of REQUIRED, SHOULD, and OPTIONAL fields, with respect to what an implementation should support and what should be in a response (using/not using response_fields).

ml-evs

Thanks for this @CasperWA, everything here looks sensible. When you're happy with it I can run through this alongside the spec to check that we have all the required fields.

Could you just clarify something for me? It's now possible for implementations to return (by default) a set of fields that would be insufficient to construct our own models from, correct? e.g. if by default an implementation does not return nsites, we cannot build an optimade-python-tools StructureResource as it nsites is required for validating the positions. If this is the case, we might have to think about producing a list somewhere of all our "required" properties so that any future client built from this repo (where the validator itself is an example) can ask for the fields it needs to build our models.

optimade/models/baseinfo.py

CasperWA · 2020-02-05T16:24:43Z

Could you just clarify something for me? It's now possible for implementations to return (by default) a set of fields that would be insufficient to construct our own models from, correct? e.g. if by default an implementation does not return nsites, we cannot build an optimade-python-tools StructureResource as it nsites is required for validating the positions. If this is the case, we might have to think about producing a list somewhere of all our "required" properties so that any future client built from this repo (where the validator itself is an example) can ask for the fields it needs to build our models.

Huh, you're right. Didn't think about this, actually. Yeah, we need to take this into account for the validator.

The `handle_response_fields` utility function now assumes that all fields to be excluded from entries are under `attributes`, since `id`, `type` and all other top-level non-attributes fields are REQUIRED in the response, even when not included as a value to `response_fields`. The mappers can use the class variable `REQUIRED_FIELDS` to have a set of REQUIRED response fields that MUST be returned, even when not included as a value to `response_fields`. Note: At the time of this commit, there are no fields under any endpoint's attributes that are REQUIRED in the response.

optimade/models/baseinfo.py

ml-evs · 2020-02-06T13:32:29Z

Added my validators and tests. I changed AnyUrl to AnyHttpUrl because you can put any garbage before :// otherwise.

CasperWA · 2020-02-06T13:56:00Z

Could you just clarify something for me? It's now possible for implementations to return (by default) a set of fields that would be insufficient to construct our own models from, correct? e.g. if by default an implementation does not return nsites, we cannot build an optimade-python-tools StructureResource as it nsites is required for validating the positions. If this is the case, we might have to think about producing a list somewhere of all our "required" properties so that any future client built from this repo (where the validator itself is an example) can ask for the fields it needs to build our models.

Huh, you're right. Didn't think about this, actually. Yeah, we need to take this into account for the validator.

Should we get this in for this PR or can it wait? I guess it can be done in a separate PR, although it is related to the same OPTiMaDe PR. This was more about the models, not the validator consequenses ...

ml-evs · 2020-02-06T13:56:47Z

Should we get this in for this PR or can it wait? I guess it can be done in a separate PR, although it is related to the same OPTiMaDe PR. This was more about the models, not the validator consequenses ...

Another PR, for sure.

@CasperWA

Bump to v0.4.0 Changes: Changes: - Reorder test files and update testing endpoints setup (#162, @CasperWA) - Separate the regular and index-meta database server, making sure they're not importing each other (#160, @CasperWA) - Introduce Starlette/FastAPI HTTP middleware for redirecting URLs ending with a slash to URLs _not_ ending with a slash (#160, @CasperWA) - Fix validation of `dimension_types` resulting in `response_fields` failing and add tests for `response_fields` (#153, @CasperWA) - Update entry list property descriptions according to updated OPTiMaDe spec (#153, @CasperWA) - Validate updated entry list properties and test updated entry list properties (#153, @ml-evs) - Add OpenAPI schema descriptions for query parameters (#166, @CasperWA) - Remove custom constrained types `NonnegativeInt` and `conlist` and use instead standard types together with `pydantic`'s `FieldInfo` parameters for schema generation and validation (#166, @shyamd, @CasperWA)

CasperWA requested a review from ml-evs February 3, 2020 12:51

ltalirz mentioned this pull request Feb 3, 2020

Checklist for OPTiMaDe v0.10.1 #29

Closed

16 tasks

CasperWA mentioned this pull request Feb 3, 2020

Minor changes to specification v0.10.1-develop #115

Closed

9 tasks

CasperWA requested a review from ltalirz February 3, 2020 16:03

ltalirz reviewed Feb 3, 2020

View reviewed changes

CasperWA changed the title ~~New REQUIRED level properties~~ [WIP] New REQUIRED level properties Feb 3, 2020

CasperWA mentioned this pull request Feb 4, 2020

Clarify required fields in a response Materials-Consortia/OPTIMADE#250

Open

CasperWA changed the title ~~[WIP] New REQUIRED level properties~~ New REQUIRED level properties Feb 4, 2020

CasperWA requested a review from ltalirz February 4, 2020 18:48

ml-evs reviewed Feb 5, 2020

View reviewed changes

optimade/models/baseinfo.py Show resolved Hide resolved

optimade/models/baseinfo.py Show resolved Hide resolved

CasperWA mentioned this pull request Feb 5, 2020

Reorder tests #162

Merged

CasperWA added 4 commits February 5, 2020 17:46

Use .get to retrieve field values in validators

5e89e3e

Update field descriptions

60d4483

Add tests for required fields with response_fields

032b7ae

CasperWA force-pushed the fix_114_new_required_level_properties branch from 7fc5adb to 032b7ae Compare February 5, 2020 16:54

CasperWA mentioned this pull request Feb 5, 2020

Add more tests #104

Closed

10 tasks

CasperWA and others added 2 commits February 6, 2020 01:04

Merge branch 'master' into fix_114_new_required_level_properties

9af8eb8

Add validator for BaseInfo.available(...).url

03e9864

CasperWA requested a review from ml-evs February 6, 2020 12:09

ml-evs reviewed Feb 6, 2020

View reviewed changes

optimade/models/baseinfo.py Show resolved Hide resolved

Crossvalidate url and version, with tests

c26c893

Blacken and make expection msg an f-string

a862084

CasperWA requested a review from ml-evs February 6, 2020 13:57

ml-evs approved these changes Feb 6, 2020

View reviewed changes

CasperWA merged commit 80e2dcc into Materials-Consortia:master Feb 6, 2020

CasperWA deleted the fix_114_new_required_level_properties branch February 6, 2020 13:57

CasperWA mentioned this pull request Feb 6, 2020

Bump to v0.4.0 #168

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New REQUIRED level properties #153

New REQUIRED level properties #153

CasperWA commented Feb 3, 2020 •

edited

Loading

ltalirz left a comment

ltalirz Feb 3, 2020

CasperWA Feb 3, 2020 •

edited

Loading

CasperWA commented Feb 4, 2020

CasperWA commented Feb 4, 2020

ml-evs left a comment

CasperWA commented Feb 5, 2020

ml-evs commented Feb 6, 2020

CasperWA commented Feb 6, 2020 •

edited

Loading

ml-evs commented Feb 6, 2020

New REQUIRED level properties #153

New REQUIRED level properties #153

Conversation

CasperWA commented Feb 3, 2020 • edited Loading

ltalirz left a comment

Choose a reason for hiding this comment

ltalirz Feb 3, 2020

Choose a reason for hiding this comment

CasperWA Feb 3, 2020 • edited Loading

Choose a reason for hiding this comment

CasperWA commented Feb 4, 2020

CasperWA commented Feb 4, 2020

ml-evs left a comment

Choose a reason for hiding this comment

CasperWA commented Feb 5, 2020

ml-evs commented Feb 6, 2020

CasperWA commented Feb 6, 2020 • edited Loading

ml-evs commented Feb 6, 2020

CasperWA commented Feb 3, 2020 •

edited

Loading

CasperWA Feb 3, 2020 •

edited

Loading

CasperWA commented Feb 6, 2020 •

edited

Loading