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

Index meta-database #103

Merged
merged 11 commits into from Dec 11, 2019
Merged

Index meta-database #103

merged 11 commits into from Dec 11, 2019

Conversation

CasperWA
Copy link
Member

@CasperWA CasperWA commented Dec 2, 2019

Closes #100

Easily implement an OPTiMaDe Index meta-database using this repository.

Start

Start the index meta-database by writing:

/path/to/optimade-python-tools$ ./run.sh index

This runs an uvicorn server at http://localhost:5001/index/optimade, making it possible to open a new terminal and also run the "standard" OPTiMaDe server (at http://localhost:5000/optimade).

Setup

The file /optimade/server/index_links_json can be modified to list all known OPTiMaDe implementations by a provider to be listed under the index meta-database's /links endpoint.

EDIT: By request from @ltalirz, I have added support for a server.cfg file in the top-package directory.
This is also added to .gitignore, so if anyone forks the repository and wished to contribute back some updates, the file will not be overwritten. Instead, a server_template.cfg file exists that should never be changed in forks, but used as a template for their own server.cfg file. This is due to the fact that if a server.cfg file is indeed not found, one will be created as a copy of server_template.cfg. This is needed for tests etc.

The point of server.cfg is to relay paths to server configuration files. So it currently has the paths for the config.ini or config.json file and the index_links.json file. Note that the paths at this point MUST be relative, with respect to the location of server.cfg. This may be updated, with a check if the paths given in server.cfg are absolute or not, before setting them in CONFIG.
The paths can be relative, but MUST be so from the location of server.cfg. They may also be absolute. They may, however, never include "~", since the Path-class cannot resolve this.

Missing

  • Make tests for the index meta-database.
  • Add OpenAPI validation for index meta-database.
  • Add comparison of the created local_index_openapi.json with the file index_openapi.json.

@codecov
Copy link

codecov bot commented Dec 2, 2019

Codecov Report

Merging #103 into master will increase coverage by 1.13%.
The diff coverage is 85.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #103      +/-   ##
==========================================
+ Coverage   85.02%   86.16%   +1.13%     
==========================================
  Files          44       47       +3     
  Lines        2371     2508     +137     
==========================================
+ Hits         2016     2161     +145     
+ Misses        355      347       -8
Flag Coverage Δ
#unittests 86.16% <85.5%> (+1.13%) ⬆️
Impacted Files Coverage Δ
optimade/server/routers/utils.py 84.03% <ø> (ø) ⬆️
optimade/models/baseinfo.py 100% <ø> (+7.69%) ⬆️
optimade/server/routers/info.py 95.83% <ø> (ø) ⬆️
optimade/validator/__init__.py 12.5% <0%> (-0.55%) ⬇️
optimade/server/tests/test_server.py 90.31% <100%> (+0.13%) ⬆️
optimade/server/tests/test_index_server.py 100% <100%> (ø)
optimade/validator/validator_model_patches.py 100% <100%> (ø) ⬆️
optimade/server/routers/index_info.py 100% <100%> (ø)
optimade/models/toplevel.py 100% <100%> (ø) ⬆️
optimade/validator/validator.py 62.99% <71.79%> (+2.26%) ⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 160951a...b411bcc. Read the comment docs.

Copy link
Member

@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.

This is looking good @CasperWA , very minor thing below.

For the sake of my understanding, could you just clarify why the index meta-db needs to be run through a separate web server? Is it just to allow the main app to run above the base url?

# raise ValueError(
# f"'{endpoint}' must be listed in available_endpoints to be valid"
# )
# return v
Copy link
Member

Choose a reason for hiding this comment

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

Why has this been removed? If it was intentional can we remove the commented out version

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 issue is class-inheritance.
This validator is run for sub-classes as well, as was intended, but it doesn't register and recognize the instance of the sub-class as that: An instance of the sub-class. Instead it thinks it's an instance of the parent class and fails.
In the pydantic docs it's writen that this can be avoided by passing the check_fields=False to the validator. But this is for pydantic v1, and didn't seem to work for me when I tested it here.
So I have commented it out, since it should be able to work for pydantic v1.

I don't know if this makes this PR blocked until we have upgraded or not, but it definitely not desireable to have a commented out model validator.

@CasperWA
Copy link
Member Author

CasperWA commented Dec 7, 2019

For the sake of my understanding, could you just clarify why the index meta-db needs to be run through a separate web server? Is it just to allow the main app to run above the base url?

Several reasons.
The index meta-database is special in that that only /info and /links are allowed endpoints. Hence, it needs a different main.py setup.
The /info is different from the normal OPTiMaDe server, and very specific to the index meta-database. Hence, I needed to create a separate router and could not reuse the current main.py by setting up some switch mechanic or other. Well maybe it was still possible, but definitely complicated things.

Finally, I thought it was nice to be able to start both servers at the same time, testing that the linking/cross-server referencing works.

@ml-evs
Copy link
Member

ml-evs commented Dec 10, 2019

For the sake of my understanding, could you just clarify why the index meta-db needs to be run through a separate web server? Is it just to allow the main app to run above the base url?

Several reasons.
The index meta-database is special in that that only /info and /links are allowed endpoints. Hence, it needs a different main.py setup.
The /info is different from the normal OPTiMaDe server, and very specific to the index meta-database. Hence, I needed to create a separate router and could not reuse the current main.py by setting up some switch mechanic or other. Well maybe it was still possible, but definitely complicated things.

Finally, I thought it was nice to be able to start both servers at the same time, testing that the linking/cross-server referencing works.

Thanks for the explanation, certainly makes sense for a reference implementation. I'll accept this once we're sure other blocking PRs are dealt with.

@CasperWA
Copy link
Member Author

Thanks for the explanation, certainly makes sense for a reference implementation. I'll accept this once we're sure other blocking PRs are dealt with.

In my opinion, this separation also works towards using this repository to directly run an index meta-database.

In any case, I think this should/can be merged whenever, preferrably before #110. But maybe we want to publish a version after this has been merged (i.e. merge #107)?

@ml-evs ml-evs self-requested a review December 11, 2019 14:55
ml-evs
ml-evs previously approved these changes Dec 11, 2019
Copy link
Member

@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.

Happy to approve this, provided we tidy up some of it in #110. Thanks @CasperWA.

@CasperWA CasperWA mentioned this pull request Dec 11, 2019
By calling `./run.sh index`, the index meta-database will be run at port
5001. This means one can run both servers on the same machine.
server_template.cfg is a template for a server.cfg file.
server.cfg is equivalent to an .ini file and can be read using
ConfigParser().
The point of server.cfg is to have a central place to write
server-specific paths to possibly changing configuration files.
At this point, it is used to direct the config.py towards the
config-file, whatever format it may be in, as well as pointing to the
index_links.json file that contains all resource objects to be added
under the index meta-database's /links endpoint.

If a server.cfg file does not exist upon loading CONFIG from config.py,
it will be created as a copy of server_template.cfg.
Warn if index_links.json cannot be found.
Raise if config-file cannot be found.
Add comparison tests for index_openapi.json.
Update pre-commit to perform tests as well.
Add test file for index meta-db test server, based on test_server.py
This is mainly to avoid creating the `server.cfg` file automatically
even when it is not needed.
Also install package .[dev] for pre-commit Actions job.
@CasperWA CasperWA merged commit ddf11f5 into master Dec 11, 2019
@CasperWA CasperWA deleted the index_meta_db branch December 11, 2019 16:39
@ltalirz ltalirz mentioned this pull request Dec 24, 2019
16 tasks
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.

Create "special" index meta-database server
2 participants