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

insert reading of default optimade_config.json in example run script run.sh #627

Merged
merged 4 commits into from Dec 14, 2020

Conversation

rartino
Copy link
Contributor

@rartino rartino commented Dec 12, 2020

This tiny fix resolves a stumbling block for new users trying out optimade-python-tools.

Without this fix, someone downloading optimade-python-tools and executing ./run.sh to try it out gets a server that otherwise works, but reports a 501 internal server error upon accessing the /structures endpoint without a a clear error description pointing the user what to do. Enabling debug mode explains that the error is a pydantic validation error due to missing chemical formula fields.

The issue is that the test data in optimade/server/data/test_structures.json absolutely needs the aliases mapping set up in the config file distributed with optimade-python-tools in /optimade_config.json. Hence, it is not possible to start a demo server without also reading that config file.

While one could consider other solutions (e.g., changing the default config hardcoded inside the python files), I think it is the expected behavior that the default example run script uses the included example config file, so that edits in that file are reflected by the running server.

@codecov
Copy link

codecov bot commented Dec 12, 2020

Codecov Report

Merging #627 (184f685) into master (f5e7b57) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #627   +/-   ##
=======================================
  Coverage   93.27%   93.27%           
=======================================
  Files          60       60           
  Lines        3256     3256           
=======================================
  Hits         3037     3037           
  Misses        219      219           
Flag Coverage Δ
project 93.27% <ø> (ø)
validator 66.89% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out 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 f5e7b57...184f685. 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.

Thanks for this @rartino! I just have a minor tweak to only use the demo value if the env var hasn't been set so that run.sh can still be used generally.

Please let us know if you run into any other problems!

run.sh Outdated Show resolved Hide resolved
Co-authored-by: Matthew Evans <7916000+ml-evs@users.noreply.github.com>
@rartino rartino requested a review from ml-evs December 12, 2020 22:59
@rartino
Copy link
Contributor Author

rartino commented Dec 12, 2020

It may be an idea to document somewhere, perhaps in a comment in run.sh and/or in a comment in server/config.py and/or in one of the readme files that any config setting can be overriden by an environment variable named OPTIMADE_<config setting>. I had to dig pretty far until I realized how that feature works since it is provided automatically by uvicorn and the env_prefix = "optimade_" in config.py.

(On the other hand, the most crucial config setting to override is the one that points to the config file to read, which is now at least explicitly demonstrated in run.sh)

@ml-evs
Copy link
Member

ml-evs commented Dec 13, 2020

It may be an idea to document somewhere, perhaps in a comment in run.sh and/or in a comment in server/config.py and/or in one of the readme files that any config setting can be overriden by an environment variable named OPTIMADE_<config setting>. I had to dig pretty far until I realized how that feature works since it is provided automatically by uvicorn and the env_prefix = "optimade_" in config.py.

(On the other hand, the most crucial config setting to override is the one that points to the config file to read, which is now at least explicitly demonstrated in run.sh)

We have this section in our documentation (https://www.optimade.org/optimade-python-tools/configuration/#environment-variables) but perhaps it would be nice to link to it in the script itself, I agree the distinction between uvicorn flags/OPTIMADE options is a bit confusing.

A docs overhaul is bubbling up to the top of my priorities for this package...

@CasperWA
Copy link
Member

CasperWA commented Dec 13, 2020

A docs overhaul is bubbling up to the top of my priorities for this package...

Yeah, an overhaul/update and some PR for it.

CasperWA
CasperWA previously approved these changes Dec 13, 2020
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.

Looks good, this. Thanks @rartino !

@rartino
Copy link
Contributor Author

rartino commented Dec 13, 2020

We have this section in our documentation (https://www.optimade.org/optimade-python-tools/configuration/#environment-variables) but perhaps it would be nice to link to it in the script itself, I agree the distinction between uvicorn flags/OPTIMADE options is a bit confusing.

Ah, thanks, I had indeed missed this. In fact, I had missed that there was a pre-rendered version of the full docs at https://www.optimade.org/optimade-python-tools/, (but I see I could have picked up the configuration info from reading `docs/configuration.md".)

Perhaps in the overhaul of the docs once could consider a slightly more prominent link to https://www.optimade.org/optimade-python-tools/ in the README.md to help direct those who glances over it, e.g. as:

# Documentation 

Full documentation is available at the [optimade-python-tools website.](https://www.optimade.org/optimade-python-tools/)

(But I see how this link gets a bit weird if you want the first page of the website to be rendered from the README.md, hmm...)

run.sh Outdated Show resolved Hide resolved
CasperWA
CasperWA previously approved these changes Dec 14, 2020
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 works for me, but I'll let @ml-evs get the final say (and merge rights) on this.
Thanks @rartino 👍

run.sh Show resolved Hide resolved
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.

Thanks again @rartino, this is really helpful.

I just added a link to the config docs in the fallback print out so hopefully that is clearer too.

@ml-evs ml-evs merged commit 5461237 into Materials-Consortia:master Dec 14, 2020
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.

None yet

3 participants