-
Couldn't load subscription status.
- Fork 49
Global state to app.state to enable multiple APIs in a single process
#2313
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2313 +/- ##
==========================================
- Coverage 90.66% 90.65% -0.01%
==========================================
Files 75 75
Lines 4980 5021 +41
==========================================
+ Hits 4515 4552 +37
- Misses 465 469 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cc34495 to
2ade55d
Compare
|
@ml-evs i think this is ready for review. The only thing failing is the build of |
|
You should be able to ignore the validator action errors, I'll make an effort to merge some thing on that end today and we'll see if this turns green. |
aa93eac to
64ee79e
Compare
|
(Just rebased with the changes from #2308) |
55a8f62 to
01fbae2
Compare
Somehow the validator now touches the server config when called via the validator action, whereas before it didn't (and thus didnt need yaml). Might have to be careful when unpicking that one... |
| from typing import Any, Literal | ||
|
|
||
| from optimade.models.entries import EntryResource | ||
| from optimade.server.config import ServerConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here's what was causing the validator failures
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks! i'm not fully understanding why but i guess it's not important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously the config import was nested under the methods that needed it, so even if the validator-action imported the mappers, it never used them so it didn't import the config. Now just importing the config class triggers import yaml at the top of server.config so we hit issues. I'm not against just making yaml required for non-server stuff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I see. yep, i don't really feel strongly about this, so we can maybe keep it as it's now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think likely this can even be removed from the validator as part of a future separation, so yeah don't think there's anything to change here. Generally, I can't see anything obviously wrong with this PR but will need to test.
Would you able to try this out on your end with MCA on this before we merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, sure, yes, I can test it out a bit more for MC deployments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @eimrek, got about halfway through a review.
Some general comments:
- The mapper stuff is definitely a breaking change (at least for my usage in odbx etc.) and I'd want to double-check the effect on performance before merging (i.e., needing to create a mapper instance every request just to parse the query params). If its minimal (likely it is) then I'm happy to include this as it does simplify things.
- This PR removes the functionality for
CONFIG.VALIDATE_API_RESPONSE, which was added to allow non-compliant databases to still host their best guess at OPTIMADE data, for example NOMAD including a "D" in chemical formulae. I'm not sure if its possible to override this in a way that works with the new config (e.g., if there's a FastAPI-native setting we can use to disable validation on a per API basis) but I think this is still required. - Can you also write a little docs page about this under the "Deployment" section for running multiple APIs that use different dbs/collections?
optimade/server/logger.py
Outdated
| LOGGER = logging.getLogger("optimade") | ||
| LOGGER.setLevel(logging.DEBUG) | ||
|
|
||
| CONFIG = ServerConfig() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably this means you can't define per-API log config? I guess the import error stuff will never trigger either. I'm not against simplifying all this by not exposing LOGGER
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this probably should be improved. it would be great to have a per-api log. I am not fully sure how to do this the best way, i can look into it.
optimade/server/query_params.py
Outdated
| if prefix in BaseResourceMapper().SUPPORTED_PREFIXES: | ||
| errors.append(param) | ||
| elif prefix not in BaseResourceMapper.KNOWN_PROVIDER_PREFIXES: | ||
| elif prefix not in BaseResourceMapper().KNOWN_PROVIDER_PREFIXES: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like that we have to initialize a class per request just to check query parameters, I'd probably prefer needing to pass the app config into this method instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops, yes, this doesn't look correct either... i'll pass in the BaseResourceMapper instance perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ended up storing the base_resource_mapper in app.state. so that the cache is used as well.
| ge=0, | ||
| ), | ||
| ] = CONFIG.page_limit, | ||
| ] = 20, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine, as long as the configured default page limit is enforced elsewhere
| if CONFIG.validate_api_response | ||
| else dict[str, Any], | ||
| response_model=ReferenceResponseMany, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually quite important to keep, not sure how to approach it with the new config mechanism
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I could retain this by doing CONFIG = ServerConfig() at the start of this module. This basically means that you take the default config (as before) for these global settings. But allow to override (other things) per-api config, if needed.
84205f9 to
65c5b64
Compare
|
@ml-evs I think now it's in a better shape. Would be great to have another review. The idea is that the main way of using is still the same - config is read either from the ENV variables or the JSON file. However, now, with the I also re-did logging such that you can specify a tag that allows one to differentiate logs from different apps. Finally, I also updates a bit the docs and added some description on how to run multiple apps. |
0c50466 to
699e6f4
Compare
This reverts commit 92fd22d.
Co-authored-by: Matthew Evans <[email protected]>
Co-authored-by: Matthew Evans <[email protected]>
Co-authored-by: Matthew Evans <[email protected]>
db845b3 to
287ef4e
Compare
Fixes #2307
This PR moves the global
CONFIGsingleton andENTRY_COLLECTIONSto the Fastapiapp.state. This required quite a large refactor, but the default functionality should be retained (so it's fully backwards compatible).Now, there is the
create_appfunction that allows to create multiple apps with different configs. So one could use something likewhich starts two different optimade APIs and an index metadb API.