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

Updated schemas to the latest generated by optimade-python-tools #315

Merged
merged 3 commits into from
Oct 30, 2020

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Sep 1, 2020

This draft PR updates the OpenAPI schemas with the latest versions generated by optimade-python-tools. I'm going to collect and explain some of the changes in turn as comments on the schema itself, so any required discussions can branch off.

As a high-level summary, we have made the following changes to the schema since 1.0:

  1. Fixed OpenAPI non-compliance (closes Swagger OpenAPI validator yields an error #314) by switching away from Python tuples and back to constrained list representations of dimension_types and lattice_vectors.
  2. Fixed an inconsistency that caused lattice_vectors and nperiodic_dimensions to not appear as required keys, as all "SHOULD"-level fields currently are (more discussion about how to handle this over at Review the required properties of StructureResourceAttributes in openapi.json optimade-python-tools#198, it should also be discussed in person at the next meeting).
  3. Updates to pydantic have made the schema cleaner, replacing single-valued "allOf" conditions with just the value, and moving many definitions to $refs.
  4. Added various keys that were added just before 1.0 release, e.g. the api_hint query parameter and aggregate, and tidied up various others, e.g. the headers for the versions endpoint.
  5. Added some missing constant types (e.g. "structures") that were not added to the schema by pydantic. They are now represented with regexps that match only "structures", "links" etc.
  6. In a few places we now allow Link objects as well as string representations of URLs, which is now reflected in our schema.

Still to change before merging:

  • We still need to fix how we handle "SHOULD" properties from the specification. In our models, this will mean making almost every field Optional, which will have the equivalent knock-on effect in the OpenAPI schema.

Co-authored-by: Casper Welzel Andersen <CasperWA@noreply.users.github.com>
Copy link
Member Author

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've hopefully been quite thorough in explaining away inconsequential changes, and in explaining the ones we intended to make, please ask if anything isn't clear

schemas/index_openapi_schema.json Outdated Show resolved Hide resolved
"attributes",
"relationships"
],
"type": "object",
"properties": {
"id": {
"title": "Id",
"pattern": "^/$",
Copy link
Member Author

@ml-evs ml-evs Sep 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and throughout, any field that has a new "pattern" property represents a field that we had previously marked as constant in pydantic, but pydantic was not generating any schema code to fix it to that value, e.g. the id here should always be "/".

@@ -947,10 +960,21 @@
},
"description": "A link **MUST** be represented as either: a string containing the link's URL or a link object."
},
"LinkType": {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The LinkType definition has simply been pulled out from an inline definition below, nothing else should have changed

@@ -890,6 +957,16 @@
},
"components": {
"schemas": {
"Aggregate": {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Aggregate definition has simply been pulled out from an inline definition below, nothing else should have changed

@@ -918,7 +995,7 @@
"description": "Statistical probability of each group. It MUST have the same length as `sites_in_groups`.\nIt SHOULD sum to one.\nSee below for examples of how to specify the probability of the occurrence of a vacancy.\nThe possible reasons for the values not to sum to one are the same as already specified above for the `concentration` of each `species`."
}
},
"description": "A description of groups of sites that are statistically correlated.\n\n- **Examples** (for each entry of the assemblies list):\n - `{\"sites_in_groups\": [[0], [1]], \"group_probabilities: [0.3, 0.7]}`: the first site and the second site never occur at the same time in the unit cell.\n Statistically, 30 % of the times the first site is present, while 70 % of the times the second site is present.\n - `{\"sites_in_groups\": [[1,2], [3]], \"group_probabilities: [0.3, 0.7]}`: the second and third site are either present together or not present; they form the first group of atoms for this assembly.\n The second group is formed by the fourth site. Sites of the first group (the second and the third) are never present at the same time as the fourth site.\n 30 % of times sites 1 and 2 are present (and site 3 is absent); 70 % of times site 3 is present (and sites 1 and 2 are absent).\n\n "
"description": "A description of groups of sites that are statistically correlated.\n\n- **Examples** (for each entry of the assemblies list):\n - `{\"sites_in_groups\": [[0], [1]], \"group_probabilities: [0.3, 0.7]}`: the first site and the second site never occur at the same time in the unit cell.\n Statistically, 30 % of the times the first site is present, while 70 % of the times the second site is present.\n - `{\"sites_in_groups\": [[1,2], [3]], \"group_probabilities: [0.3, 0.7]}`: the second and third site are either present together or not present; they form the first group of atoms for this assembly.\n The second group is formed by the fourth site. Sites of the first group (the second and the third) are never present at the same time as the fourth site.\n 30 % of times sites 1 and 2 are present (and site 3 is absent); 70 % of times site 3 is present (and sites 1 and 2 are absent)."
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a whitespace change brought about by updates to our automatic code formatter (deleted trailing new lines and spaces).

@@ -2780,7 +2820,7 @@
"representation": {
"title": "Representation",
"type": "string",
"description": "A string with the part of the URL following the versioned or unversioned base URL that serves the API.\nQuery parameters that have not been used in processing the request MAY be omitted.\nIn particular, if no query parameters have been involved in processing the request, the query pary of the URL MAY be excluded.\nExample: `/structures?filter=nelements=2`"
"description": "A string with the part of the URL following the versioned or unversioned base URL that serves the API.\nQuery parameters that have not been used in processing the request MAY be omitted.\nIn particular, if no query parameters have been involved in processing the request, the query part of the URL MAY be excluded.\nExample: `/structures?filter=nelements=2`"
Copy link
Member Author

@ml-evs ml-evs Sep 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed a typo introduced in our mad rush for 1.0 release...

"description": "A list describing the species of the sites of this structure.\nSpecies can represent pure chemical elements, virtual-crystal atoms representing a\nstatistical occupation of a given site by multiple chemical elements, and/or a\nlocation to which there are attached atoms, i.e., atoms whose precise location are\nunknown beyond that they are attached to that position (frequently used to indicate\nhydrogen atoms attached to another element, e.g., a carbon with three attached\nhydrogens might represent a methyl group, -CH3).\n\n- **Examples**:\n - `[ {\"name\": \"Ti\", \"chemical_symbols\": [\"Ti\"], \"concentration\": [1.0]} ]`: any site with this species is occupied by a Ti atom.\n - `[ {\"name\": \"Ti\", \"chemical_symbols\": [\"Ti\", \"vacancy\"], \"concentration\": [0.9, 0.1]} ]`: any site with this species is occupied by a Ti atom with 90 % probability, and has a vacancy with 10 % probability.\n - `[ {\"name\": \"BaCa\", \"chemical_symbols\": [\"vacancy\", \"Ba\", \"Ca\"], \"concentration\": [0.05, 0.45, 0.5], \"mass\": 88.5} ]`: any site with this species is occupied by a Ba atom with 45 % probability, a Ca atom with 50 % probability, and by a vacancy with 5 % probability. The mass of this site is (on average) 88.5 a.m.u.\n - `[ {\"name\": \"C12\", \"chemical_symbols\": [\"C\"], \"concentration\": [1.0], \"mass\": 12.0} ]`: any site with this species is occupied by a carbon isotope with mass 12.\n - `[ {\"name\": \"C13\", \"chemical_symbols\": [\"C\"], \"concentration\": [1.0], \"mass\": 13.0} ]`: any site with this species is occupied by a carbon isotope with mass 13.\n - `[ {\"name\": \"CH3\", \"chemical_symbols\": [\"C\"], \"concentration\": [1.0], \"attached\": [\"H\"], \"nattached\": [3]} ]`: any site with this species is occupied by a methyl group, -CH3, which is represented without specifying precise positions of the hydrogen atoms.\n\n "
"description": "A list describing the species of the sites of this structure.\n\nSpecies can represent pure chemical elements, virtual-crystal atoms representing a\nstatistical occupation of a given site by multiple chemical elements, and/or a\nlocation to which there are attached atoms, i.e., atoms whose precise location are\nunknown beyond that they are attached to that position (frequently used to indicate\nhydrogen atoms attached to another element, e.g., a carbon with three attached\nhydrogens might represent a methyl group, -CH3).\n\n- **Examples**:\n - `[ {\"name\": \"Ti\", \"chemical_symbols\": [\"Ti\"], \"concentration\": [1.0]} ]`: any site with this species is occupied by a Ti atom.\n - `[ {\"name\": \"Ti\", \"chemical_symbols\": [\"Ti\", \"vacancy\"], \"concentration\": [0.9, 0.1]} ]`: any site with this species is occupied by a Ti atom with 90 % probability, and has a vacancy with 10 % probability.\n - `[ {\"name\": \"BaCa\", \"chemical_symbols\": [\"vacancy\", \"Ba\", \"Ca\"], \"concentration\": [0.05, 0.45, 0.5], \"mass\": 88.5} ]`: any site with this species is occupied by a Ba atom with 45 % probability, a Ca atom with 50 % probability, and by a vacancy with 5 % probability. The mass of this site is (on average) 88.5 a.m.u.\n - `[ {\"name\": \"C12\", \"chemical_symbols\": [\"C\"], \"concentration\": [1.0], \"mass\": 12.0} ]`: any site with this species is occupied by a carbon isotope with mass 12.\n - `[ {\"name\": \"C13\", \"chemical_symbols\": [\"C\"], \"concentration\": [1.0], \"mass\": 13.0} ]`: any site with this species is occupied by a carbon isotope with mass 13.\n - `[ {\"name\": \"CH3\", \"chemical_symbols\": [\"C\"], \"concentration\": [1.0], \"attached\": [\"H\"], \"nattached\": [3]} ]`: any site with this species is occupied by a methyl group, -CH3, which is represented without specifying precise positions of the hydrogen atoms."
},
"StructureFeatures": {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StructureFeatures definition moved, no changes

@@ -2841,7 +2881,17 @@
"description": "If provided MUST be a list of length 1 or more of integers indicating the number of attached atoms of the kind specified in the value of the :field:`attached` key."
}
},
"description": "A list describing the species of the sites of this structure.\nSpecies can represent pure chemical elements, virtual-crystal atoms representing a\nstatistical occupation of a given site by multiple chemical elements, and/or a\nlocation to which there are attached atoms, i.e., atoms whose precise location are\nunknown beyond that they are attached to that position (frequently used to indicate\nhydrogen atoms attached to another element, e.g., a carbon with three attached\nhydrogens might represent a methyl group, -CH3).\n\n- **Examples**:\n - `[ {\"name\": \"Ti\", \"chemical_symbols\": [\"Ti\"], \"concentration\": [1.0]} ]`: any site with this species is occupied by a Ti atom.\n - `[ {\"name\": \"Ti\", \"chemical_symbols\": [\"Ti\", \"vacancy\"], \"concentration\": [0.9, 0.1]} ]`: any site with this species is occupied by a Ti atom with 90 % probability, and has a vacancy with 10 % probability.\n - `[ {\"name\": \"BaCa\", \"chemical_symbols\": [\"vacancy\", \"Ba\", \"Ca\"], \"concentration\": [0.05, 0.45, 0.5], \"mass\": 88.5} ]`: any site with this species is occupied by a Ba atom with 45 % probability, a Ca atom with 50 % probability, and by a vacancy with 5 % probability. The mass of this site is (on average) 88.5 a.m.u.\n - `[ {\"name\": \"C12\", \"chemical_symbols\": [\"C\"], \"concentration\": [1.0], \"mass\": 12.0} ]`: any site with this species is occupied by a carbon isotope with mass 12.\n - `[ {\"name\": \"C13\", \"chemical_symbols\": [\"C\"], \"concentration\": [1.0], \"mass\": 13.0} ]`: any site with this species is occupied by a carbon isotope with mass 13.\n - `[ {\"name\": \"CH3\", \"chemical_symbols\": [\"C\"], \"concentration\": [1.0], \"attached\": [\"H\"], \"nattached\": [3]} ]`: any site with this species is occupied by a methyl group, -CH3, which is represented without specifying precise positions of the hydrogen atoms.\n\n "
"description": "A list describing the species of the sites of this structure.\n\nSpecies can represent pure chemical elements, virtual-crystal atoms representing a\nstatistical occupation of a given site by multiple chemical elements, and/or a\nlocation to which there are attached atoms, i.e., atoms whose precise location are\nunknown beyond that they are attached to that position (frequently used to indicate\nhydrogen atoms attached to another element, e.g., a carbon with three attached\nhydrogens might represent a methyl group, -CH3).\n\n- **Examples**:\n - `[ {\"name\": \"Ti\", \"chemical_symbols\": [\"Ti\"], \"concentration\": [1.0]} ]`: any site with this species is occupied by a Ti atom.\n - `[ {\"name\": \"Ti\", \"chemical_symbols\": [\"Ti\", \"vacancy\"], \"concentration\": [0.9, 0.1]} ]`: any site with this species is occupied by a Ti atom with 90 % probability, and has a vacancy with 10 % probability.\n - `[ {\"name\": \"BaCa\", \"chemical_symbols\": [\"vacancy\", \"Ba\", \"Ca\"], \"concentration\": [0.05, 0.45, 0.5], \"mass\": 88.5} ]`: any site with this species is occupied by a Ba atom with 45 % probability, a Ca atom with 50 % probability, and by a vacancy with 5 % probability. The mass of this site is (on average) 88.5 a.m.u.\n - `[ {\"name\": \"C12\", \"chemical_symbols\": [\"C\"], \"concentration\": [1.0], \"mass\": 12.0} ]`: any site with this species is occupied by a carbon isotope with mass 12.\n - `[ {\"name\": \"C13\", \"chemical_symbols\": [\"C\"], \"concentration\": [1.0], \"mass\": 13.0} ]`: any site with this species is occupied by a carbon isotope with mass 13.\n - `[ {\"name\": \"CH3\", \"chemical_symbols\": [\"C\"], \"concentration\": [1.0], \"attached\": [\"H\"], \"nattached\": [3]} ]`: any site with this species is occupied by a methyl group, -CH3, which is represented without specifying precise positions of the hydrogen atoms."
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whitespace changes only

@@ -3045,69 +3075,29 @@
},
"lattice_vectors": {
"title": "Lattice Vectors",
"maxItems": 3,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change (and the one for dimension_types) relates to point 1 above, we are now OpenAPI compliant and are validating against validator.swagger.io in our CI.

@@ -3445,11 +3419,6 @@
],
"description": "A links object storing about"
},
"status": {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't see why this would have disappeared, any ideas @CasperWA?

Copy link
Member

@CasperWA CasperWA Sep 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. status is not part of the OPTIMADE Warning data structure. See the details under warnings here. So accordingly, it has been removed from the schema.

The removal is hard-coded as a post-schema creation thing in the pydantic model for Warning. See here.

Comment on lines +3004 to +3005
"nperiodic_dimensions",
"lattice_vectors",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From point 2 above:

Fixed an inconsistency that caused lattice_vectors and nperiodic_dimensions to not appear as required keys, as all "SHOULD"-level fields currently are (more discussion about how to handle this over at Materials-Consortia/optimade-python-tools#198, it should also be discussed in person at the next meeting).

@ml-evs
Copy link
Member Author

ml-evs commented Sep 14, 2020

Pending a discussion of whether "SHOULD" fields in the specification should be marked as "required" keys under OpenAPI, the rest of the changes here can now be reviewed and (hopefully) merged.

@ml-evs ml-evs marked this pull request as ready for review September 14, 2020 21:52
@ml-evs ml-evs added PR/ready-for-review Add this flag if you are the author of the PR and you want it to be reviewed. Remove it when editing priority/mid There is a consensus that this have average priority to sort out. labels Sep 14, 2020
Co-authored-by: Casper Welzel Andersen <casper.andersen@epfl.ch>
Copy link
Member

@merkys merkys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@ml-evs
Copy link
Member Author

ml-evs commented Oct 30, 2020

Following the discussion yesterday, I'm going to merge this PR in ~1 hour (unless there are disagreements) and start preparing another PR with our latest updates from optimade-python-tools (regexp for chemical formulae, SHOULD fields being listed as nullable).

@ml-evs ml-evs merged commit 250a95c into develop Oct 30, 2020
@ml-evs ml-evs deleted the python-tools-schema-updates branch October 30, 2020 17:56
ml-evs added a commit that referenced this pull request Feb 16, 2021
* Updated schemas to the latest generated by optimade-python-tools v0.12.0

Co-authored-by: Casper Welzel Andersen <casper.andersen@epfl.ch>
ml-evs added a commit that referenced this pull request Feb 26, 2021
* Updated schemas to the latest generated by optimade-python-tools v0.12.0

Co-authored-by: Casper Welzel Andersen <casper.andersen@epfl.ch>
ml-evs added a commit that referenced this pull request May 30, 2021
* Updated schemas to the latest generated by optimade-python-tools v0.12.0

Co-authored-by: Casper Welzel Andersen <casper.andersen@epfl.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR/ready-for-review Add this flag if you are the author of the PR and you want it to be reviewed. Remove it when editing priority/mid There is a consensus that this have average priority to sort out.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Swagger OpenAPI validator yields an error
4 participants