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

add 2DMatpedia to the providers, with a static index meta-db #74

Merged
merged 2 commits into from
Sep 10, 2021

Conversation

migueldiascosta
Copy link
Contributor

No description provided.

@CasperWA
Copy link
Member

First, thank you for this PR to add in 2DMatpedia! :)

I've checked out your implementation at http://optimade.2dmatpedia.org and there are some things that should probably be updated:

  • The meta.implementation object.
    This can be changed by updating the config.json file with the appropriate information.
  • The /references endpoint data.
    It is still using the test data from the OPTIMADE Python tools example implementation.
    If you don't have any references data, you can simply not load data for this endpoint - that's completely fine. You can even avoid having the endpoint altogether, however, I'd recommend keeping it, but just avoid to load and use the test data. You might also be loading the test data for the /structures endpoint, please check and avoid this as well if this is the case.

Concerning this specific PR I don't see any issues other than the incommensurate use of the provider prefix in your implementation.

Copy link
Contributor Author

@migueldiascosta migueldiascosta left a comment

Choose a reason for hiding this comment

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

to clarify the "test" data, I wanted to at least at first try using the mongomock backend using a static json file, and since I didn't see a way of providing a file, I merely replaced the optimade/server/data/test_structures.json file. So yes, insert_test_data is true, but that test data is an export from 2dmatpedia, adapted to the optimade specification

src/index-metadbs/2dmatpedia/v1/links.json Outdated Show resolved Hide resolved
@CasperWA
Copy link
Member

to clarify the "test" data, I wanted to at least at first try using the mongomock backend using a static json file, and since I didn't see a way of providing a file, I merely replaced the optimade/server/data/test_structures.json file. So yes, insert_test_data is true, but that test data is an export from 2dmatpedia, adapted to the optimade specification

Right.
Yeah, so the way you would do this in production is to have a MongoDB database service running, which you would connect to. It should then contain the data in a similar fashion as the test data file. If you want to use different collection names than the default ones, you can change the configurations for structures_collection and references_collection, etc. You should then also make sure to change the configuration for mongo_uri to point to your MongoDB database service.

If 2DMatpedia does not actually use MongoDB in the backend and the data is stored in a different kind of database you might need to setup your server differently, with a different EntryCollection class and Transformer class, which transforms the OPTIMADE filter query into a backend-specific query.

If you want more assistance with this let either me, @ml-evs or @JPBergsma know and we will try to find some time to have a meeting to discuss how OPTIMADE Python tools can most effeciently be utilized for your implementation :)

For now, you can surely do it the way you have - I would recommend to just avoid the server loading the /references test data. Either by updating the test_references.json file by making it an empty list or editing the Python code to avoid loading data for this endpoint.

@migueldiascosta
Copy link
Contributor Author

If 2DMatpedia does not actually use MongoDB in the backend

it does (it was built with the materials project tools), but since I had to "massage" some data to make it conform to the optimade specification, it was simpler to do it on a file

For now, you can surely do it the way you have - I would recommend to just avoid the server loading the /references test data. Either by updating the test_references.json file by making it an empty list or editing the Python code to avoid loading data for this endpoint.

I've just added the 2dmatpedia publication there

The meta.implementation object.
This can be changed by updating the config.json file with the appropriate information.

I am indeed using the implementation from optimade-python-tools, without any modifications, so that information is correct, no?

@CasperWA
Copy link
Member

If 2DMatpedia does not actually use MongoDB in the backend

it does (it was built with the materials project tools), but since I had to "massage" some data to make it conform to the optimade specification, it was simpler to do it on a file

Fair enough. This can be remedied by a special Mapper class, in which case you could connect directly to you existing data and serve it with the OPTIMADE data models, but maybe let's talk about that in the suggested meeting, if you want?

For now, you can surely do it the way you have - I would recommend to just avoid the server loading the /references test data. Either by updating the test_references.json file by making it an empty list or editing the Python code to avoid loading data for this endpoint.

I've just added the 2dmatpedia publication there

Excellent.

The meta.implementation object.
This can be changed by updating the config.json file with the appropriate information.

I am indeed using the implementation from optimade-python-tools, without any modifications, so that information is correct, no?

Well, I'm guessing you're using a fork (or you should) or similar? Which you can then link to - maybe even the specific branch?
This is also important if there should be any issues reported that are 2DMatpedia-specific. They shouldn't be directed to the general OPTIMADE Python tools repository, unless of course it is an issue with the Python tools specifically.

As an example, it might be that you have calculated/converted a value of your structures wrongly, which is a specific 2DMatpedia issue, not an OPTIMADE Python tools issue :)

@migueldiascosta
Copy link
Contributor Author

Ok, I'll point that to the fork :)

Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

You can also update the id in the links.json file if you wish, but it needs to be updated where I made the suggestion to become the official provider prefix.

},
{
"type": "links",
"id": "2dmatpedia",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"id": "2dmatpedia",
"id": "twodmatpedia",

This represents the provider prefix as well :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what about the folder name, does it also need to be twodmatpedia then?

Copy link
Member

Choose a reason for hiding this comment

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

Hi @migueldiascosta, that might also make sense! Good to see that we have our first edge case in not allowing numbers in the prefix...

Also thanks for making this implementation with our tools! As @CasperWA suggested we are more than happy to meet to help streamline the process and make the package more ergonomic.

@CasperWA CasperWA marked this pull request as ready for review September 10, 2021 10:42
Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

This is all good to go in my opinion - again, thank you for this contribution @migueldiascosta ! 👍

If at some point in the future you change from http to https remember to come back here and update the links.json accordingly :)

Copy link
Contributor

@rartino rartino left a comment

Choose a reason for hiding this comment

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

Nice to see a new provider! Welcome!

@rartino rartino merged commit 357c27b into Materials-Consortia:master Sep 10, 2021
@migueldiascosta
Copy link
Contributor Author

thanks (although I had opened this as a draft PR... guess we'll need to step up our efforts to improve what we are providing via optimade - maybe that was the point? :p )

btw, is that static index-metadb service down? all of those links in https://www.optimade.org/providers-dashboard/ return 404

(I did test running our own index with the optimade-python-tools and it seems to work, and we do have a few related databases, but at least for now we'd prefer using your static service, if possible)

@migueldiascosta
Copy link
Contributor Author

migueldiascosta commented Sep 12, 2021

ah, ok, adding 'v1/links' to the url works (but then the link in the dashboard should point to something more informative? or just be the base_url string, not a clickable link...)

@CasperWA
Copy link
Member

thanks (although I had opened this as a draft PR... guess we'll need to step up our efforts to improve what we are providing via optimade - maybe that was the point? :p )

Yeah I bumped it up (sorry). But several of the other providers here are already in a similar state. It's better to also register (and get it merged in) before having the fully fledged implementation on your side ready.

In the end you can utilize the aggregate attribute for a LinksResource in you index meta-db and change it from "ok" to "staging" or "test" to indicate it is not yet fully in production? Although this will only be a friendly indicator and doesn't come with actual practical gates for how to access your implementation.

btw, is that static index-metadb service down? all of those links in https://www.optimade.org/providers-dashboard/ return 404

(I did test running our own index with the optimade-python-tools and it seems to work, and we do have a few related databases, but at least for now we'd prefer using your static service, if possible)

That's what the service is here for :) You can always come back and update the base_url attribute for your entry in the providers list.

@CasperWA
Copy link
Member

CasperWA commented Sep 12, 2021

ah, ok, adding 'v1/links' to the url works (but then the link in the dashboard should point to something more informative? or just be the base_url string, not a clickable link...)

Good point. I'll add an issue concerning this.

Edit: Created issue here: Materials-Consortia/providers-dashboard#60.

@CasperWA
Copy link
Member

Also, as a note, I have updated the OPTIMADE Client hosted by Materials Cloud to support "minimized" structures, i.e., where not all structural information is valid, which was the case for you structures to begin with, I believe they have had more information added since then now - but you can peruse your OPTIMADE implementation through that client at https://materialscloud.org/optimadeclient :)

@migueldiascosta
Copy link
Contributor Author

Also, as a note, I have updated the OPTIMADE Client hosted by Materials Cloud to support "minimized" structures, i.e., where not all structural information is valid, which was the case for you structures to begin with, I believe they have had more information added since then now

yeah, I had started with the required ones, and then noticed that lattice_vectors was still missing. The database was created with https://atomate.org/ and related tools pymatgen, custodian, and FireWorks (when we started the project, quite a few years ago now, we weren't aware of Aiida), there may be a case for supporting a default mapping? (e.g. besides finding the lattice_vectors matrix inside the structure, if it looked for bandgap as well as band_gap, it would have filled in _twodmatpedia_band_gap in the first place, and other small differences).

Cool. Although I assumed this was the point, I wasn't aware that it already existed :)

@ml-evs
Copy link
Member

ml-evs commented Sep 13, 2021

ah, ok, adding 'v1/links' to the url works (but then the link in the dashboard should point to something more informative? or just be the base_url string, not a clickable link...)

This catches me out all the time... I would be very happy to add redirects or a more informative 404 page, hopefully this is not too difficult with our current netlify setup.

EDIT: although the problem is present when clicking on the links in the dashboard, I think the main problem is really that the static index meta-dbs hosted here do not have nice 404/landing pages, whereas most other implementations do (or at least they have a 404 that looks like an OPTIMADE implementation).

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