Using routers #99

CasperWA · 2019-11-28T15:25:59Z

All endpoints are now specified by FastAPI's APIRouter class.
This means that we can create the same endpoints several times, which is useful for the different base URL's required by the OPTiMaDe specification, e.g., /optimade, /optimade/v0.10, ...

All endpoints are now specified by FastAPI's APIRouter class. This means that we can create the same endpoints several times, which is useful for the different base URL's required by the OPTiMaDe specification, e.g., /optimade, /optimade/v0.10, ...

codecov · 2019-11-28T15:26:30Z

Codecov Report

Merging #99 into master will increase coverage by 0.36%.
The diff coverage is 79.25%.

@@            Coverage Diff             @@
##           master      #99      +/-   ##
==========================================
+ Coverage   84.99%   85.36%   +0.36%     
==========================================
  Files          39       45       +6     
  Lines        2352     2425      +73     
==========================================
+ Hits         1999     2070      +71     
- Misses        353      355       +2

Impacted Files	Coverage Δ
optimade/models/utils.py	`87.5% <ø> (ø)`
optimade/models/baseinfo.py	`92.59% <ø> (ø)`	⬆️
optimade/validator/__init__.py	`13.04% <ø> (ø)`	⬆️
optimade/server/routers/utils.py	`84.03% <100%> (ø)`
optimade/models/tests/test_models.py	`100% <100%> (ø)`	⬆️
optimade/models/structures.py	`83.33% <100%> (ø)`	⬆️
optimade/server/routers/__init__.py	`100% <100%> (ø)`
optimade/server/routers/structures.py	`100% <100%> (ø)`
optimade/models/__init__.py	`100% <100%> (ø)`	⬆️
optimade/server/main.py	`93.87% <100%> (+19.83%)`	⬆️
... and 16 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 bcd2b72...ca19f54. Read the comment docs.

ml-evs · 2019-11-28T19:39:04Z

This is an extremely useful PR @CasperWA! I was trying to figure out how to do something similar with my implementation so I could just import most of the endpoints from the example server code.

Is this ready to review?

CasperWA · 2019-11-28T19:41:18Z

This is an extremely useful PR @CasperWA! I was trying to figure out how to do something similar with my implementation so I could just import most of the endpoints from the example server code.

😄 🎉

Is this ready to review?

Sure is 👍

ml-evs

I am just as impressed after reading it as I was after skimming it, great work, definitely want to use these features ASAP (I assume this needs to be updated to include /links when #95 has been merged, and not vice versa).

optimade/server/main.py

ml-evs · 2019-11-28T20:31:17Z

optimade/server/routers/references.py

+    response_model=Union[ReferenceResponseMany, ErrorResponse],
+    response_model_skip_defaults=True,


Getting lots of warnings about resposne_model_skip_defaults from the new FastAPI, could be worth updating it here to expedite the upgrade to pydantic>1.

response_model_skip_defaults has been deprecated in favor of response_model_exclude_unset to keep in line with Pydantic v1, support for it will be removed soon.

Note: This keyword only works for fastapi>=0.44.0. So we would need to pin this version as a minimum version if we use this keyword.

That's fair, happy to leave it for big deps update PR then

ml-evs · 2019-11-28T20:34:07Z

optimade/server/routers/structures.py

+router = APIRouter()
+
+structures_coll = MongoCollection(
+    client[CONFIG.mongo_database]["structures"], StructureResource, StructureMapper


I think I'm still in favour of a CONFIG.structure_collection that defaults to "structures", but that's very minor.

I think this is very back-end specific. We can definitely add a [MONGO] section to the config, where it can be added. I also think we shouldn't use the [DEFAULT] section, since it's special for config.ini and it's fields will always be included in all other sections. This is a problem for the sections specifying provider-specific fields.

I get your point, I guess I'm just being lazy by wanting to reuse even main.py :P

Well, I have actually updated this PR with all your suggested changes - check it out :)

optimade/server/tests/test_server.py

ml-evs · 2019-11-28T20:41:56Z

optimade/server/utils.py

-        provider=Provider(**provider),
-        data_available=data_available,
-        **kwargs,
-    )


 def general_exception(


Any reason this can't be moved to exception_handlers (which I guess could renamed exceptions)? Doesn't really fit with the other utils submodules now. Also I noticed that we have models.util, server.utils and server.routers.utils, please standardize this however you see fit (if server.utils is going, probably better to rename to server.routers.util as all imports of it will already be in this PR)!

Any reason this can't be moved to exception_handlers (which I guess could renamed exceptions)?

I indeed call it this in my own implementation. But I think it's a bit misleading, since one would most likely expect Python exceptions to be the content here...

Doesn't really fit with the other utils submodules now. Also I noticed that we have models.util, server.utils and server.routers.utils, please standardize this however you see fit (if server.utils is going, probably better to rename to server.routers.util as all imports of it will already be in this PR)!

Ah, yeah. It should all be utils. And I am indeed trying to move the functions around to the appropriate places.

ml-evs · 2019-11-28T22:33:34Z

optimade/server/routers/info.py

+                api_version="v0.10",
+                available_api_versions=[
+                    {
+                        "url": "http://localhost:5000/optimade/v0.10.0/",


Can we now use CONFIG.index_base_url to figure out what this url should be?

Not a bad idea! :)

Note: This should be put here, but rather, it should be used in the top-level meta response under provider - which it is automatically :)

What I have done in my own implementation, however, is to add version to the config file. In this way it can be used here and retrieved whenever needed, without having to import the app to get the version from there.

CasperWA · 2019-11-29T10:02:15Z

(...) (I assume this needs to be updated to include /links when #95 has been merged, and not vice versa).

Yeah, I think it's more prudent to wait merging this until we've added /links, i.e., merged #95.

Up min. required dependency for fastapi to 0.44.0. This lets us begin to slowly update to be pydantic v1 ready. Change config.ini significantly, splitting it up in backend, implementation, and endpoints.

ml-evs · 2019-12-01T18:47:52Z

Thanks for these changes @CasperWA, I'll accept once we've merged #95.

ml-evs

Not sure why Travis isn't reporting success back to GitHub, maybe if you make this one change it will work again?

optimade/server/routers/links.py

ml-evs

Cheers @CasperWA! Merge when you're ready.

Using routers

b2f2d52

All endpoints are now specified by FastAPI's APIRouter class. This means that we can create the same endpoints several times, which is useful for the different base URL's required by the OPTiMaDe specification, e.g., /optimade, /optimade/v0.10, ...

CasperWA requested review from ltalirz, fekad and ml-evs November 28, 2019 15:25

CasperWA added the enhancement New feature or request label Nov 28, 2019

CasperWA mentioned this pull request Nov 28, 2019

Create "special" index meta-database server #100

Closed

Update openapi.json

cd6a8c9

ml-evs reviewed Nov 28, 2019

View reviewed changes

Address comments from @ml-evs review and more

99fc809

Up min. required dependency for fastapi to 0.44.0. This lets us begin to slowly update to be pydantic v1 ready. Change config.ini significantly, splitting it up in backend, implementation, and endpoints.

CasperWA requested a review from ml-evs December 1, 2019 15:13

ml-evs mentioned this pull request Dec 1, 2019

Use GitHub Actions for CI #92

Merged

CasperWA mentioned this pull request Dec 2, 2019

Add /links #95

Merged

CasperWA added 3 commits December 2, 2019 12:27

Merge branch 'master' into use_routers

5542556

Add /links router

d0f6470

Minor linting update

1cb6e3c

ml-evs requested changes Dec 2, 2019

View reviewed changes

optimade/server/routers/links.py Outdated Show resolved Hide resolved

skip_defaults -> exclude_unset

ca19f54

CasperWA requested a review from ml-evs December 2, 2019 13:56

ml-evs approved these changes Dec 2, 2019

View reviewed changes

CasperWA merged commit 6675838 into master Dec 2, 2019

CasperWA deleted the use_routers branch December 2, 2019 14:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using routers #99

Using routers #99

CasperWA commented Nov 28, 2019

codecov bot commented Nov 28, 2019 •

edited

Loading

ml-evs commented Nov 28, 2019

CasperWA commented Nov 28, 2019

ml-evs left a comment •

edited

Loading

ml-evs Nov 28, 2019

CasperWA Nov 29, 2019

ml-evs Nov 29, 2019

ml-evs Nov 28, 2019

CasperWA Nov 29, 2019 •

edited

Loading

ml-evs Nov 29, 2019

CasperWA Nov 29, 2019

ml-evs Nov 28, 2019

CasperWA Nov 29, 2019

ml-evs Nov 28, 2019 •

edited

Loading

CasperWA Nov 29, 2019

CasperWA Nov 29, 2019

CasperWA commented Nov 29, 2019

ml-evs commented Dec 1, 2019

ml-evs left a comment

ml-evs left a comment

		response_model=Union[ReferenceResponseMany, ErrorResponse],
		response_model_skip_defaults=True,

Using routers #99

Using routers #99

Conversation

CasperWA commented Nov 28, 2019

codecov bot commented Nov 28, 2019 • edited Loading

Codecov Report

ml-evs commented Nov 28, 2019

CasperWA commented Nov 28, 2019

ml-evs left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CasperWA Nov 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ml-evs Nov 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CasperWA commented Nov 29, 2019

ml-evs commented Dec 1, 2019

ml-evs left a comment

Choose a reason for hiding this comment

ml-evs left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 28, 2019 •

edited

Loading

ml-evs left a comment •

edited

Loading

CasperWA Nov 29, 2019 •

edited

Loading

ml-evs Nov 28, 2019 •

edited

Loading