Fix circular dep and extra permission error in logs

[ml-evs]

Closes #435.

Reworks error handling of config and logging set up.

[codecov]

Codecov Report

Merging #436 into master will decrease coverage by 0.34%.
The diff coverage is 67.74%.

@@            Coverage Diff             @@
##           master     #436      +/-   ##
==========================================
- Coverage   91.60%   91.25%   -0.35%     
==========================================
  Files          60       60              
  Lines        2800     2814      +14     
==========================================
+ Hits         2565     2568       +3     
- Misses        235      246      +11     

Flag	Coverage Δ
#unittests	91.25% <67.74%> (-0.35%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
optimade/server/logger.py	68.75% <33.33%> (-7.92%)	⬇️
optimade/server/config.py	88.05% <57.14%> (-8.96%)	⬇️
optimade/server/main.py	94.73% <85.71%> (-1.35%)	⬇️
optimade/server/main_index.py	94.73% <85.71%> (-1.35%)	⬇️

---

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 c911e75...77c059b. Read the comment docs.

[CasperWA]

Great trick, thanks @ml-evs!

If this works with the tests, then there is indeed no need to have the extra validator for log_level.

A minor comment, since I am confused about this.

[ml-evs]

Great trick, thanks @ml-evs!

If this works with the tests, then there is indeed no need to have the extra validator for log_level.

A minor comment, since I am confused about this.

My only worry is that logging.get_logger("optimade") might get called before the logger is actually set up, so the default config errors go to the wrong place? Not sure it's that a big of a deal in that case though

[CasperWA]

Great trick, thanks @ml-evs!
If this works with the tests, then there is indeed no need to have the extra validator for log_level.
A minor comment, since I am confused about this.

My only worry is that logging.get_logger("optimade") might get called before the logger is actually set up, so the default config errors go to the wrong place? Not sure it's that a big of a deal in that case though

You should be able to see this if you start the server via ./run.sh debug.
If you don't see a nicely formatted DEBUG: Found config file at ... or something like that, then it's not initializing the logger prior to this.

[CasperWA]

Great trick, thanks @ml-evs!
If this works with the tests, then there is indeed no need to have the extra validator for log_level.
A minor comment, since I am confused about this.

My only worry is that logging.get_logger("optimade") might get called before the logger is actually set up, so the default config errors go to the wrong place? Not sure it's that a big of a deal in that case though

You should be able to see this if you start the server via ./run.sh debug.
If you don't see a nicely formatted DEBUG: Found config file at ... or something like that, then it's not initializing the logger prior to this.

I just checked this. And it seems that the logger is indeed not initialized.

We could instead completely remove the log statements from config.py? It would be nice to have it logged what config file is used though. But maybe we could move that to the main*.py files? I don't know... Or perhaps just the logger.py file?

[ml-evs]

Just tweaked this some more with (hopefully) improved error handling. Only thing I can't get is for a RuntimeError to crash the server if the config is borked.

[ml-evs]

To summarise new behaviour (might not be what we actually want):

1. If OPTIMADE_CONFIG_FILE is set, then try to crash if the file isn't found (as it stands, uvicorn keeps hanging on).
2. If OPTIMADE_CONFIG_FILE is unset and ~/.optimade.json doesn't exist, emit a warning that the defaults are being used.

[CasperWA]

To summarise new behaviour (might not be what we actually want):

1. If OPTIMADE_CONFIG_FILE is set, then try to crash if the file isn't found (as it stands, uvicorn keeps hanging on).

Yeah, I think as with the logging to files, this should just produce a very visible warning (through logging) and then use the default values.

2. If OPTIMADE_CONFIG_FILE is unset and ~/.optimade.json doesn't exist, emit a warning that the defaults are being used.

Yir.

Wait, was the first point the one we now cannot do due to the circular import?

[ml-evs]

It's a little bit verbose now, but I think this is a sensible set up. Config emits warnings that are caught (and then reset) by the server and written to the main log. Any config load failures only warn and don't cause a crash, but clearly print that defaults are being used instead.

I also moved all the CONFIG importing code to the top of the server code, as it was actually being imported for the first time inside exception_handling somewhere.

[CasperWA]

Quite verbose now, yes. But in this case, I think it really makes sense.

This is great work @ml-evs ! Chanks.

I have approved with a single comment. I may have missed something, so it might indeed be important to keep that line in. If not, just ping me and I'll quickly re-approve 👍