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

fix interface of run_api for running REST API #3875

Merged
merged 1 commit into from
Mar 29, 2020

Conversation

ltalirz
Copy link
Member

@ltalirz ltalirz commented Mar 28, 2020

Fixes #3870

The current run_api interface for running the AiiDA REST API had
unnecessarily many required arguments, making it complicated to use in WSGI
scripts, e.g.:

from aiida.restapi import api
from aiida.restapi.run_api import run_api
import aiida.restapi

CONFIG_DIR = os.path.join(os.path.split(
                  os.path.abspath(aiida.restapi.__file__))[0], 'common')

(app, api) = run_api(
    api.App,
    api.AiidaApi,
    hostname="localhost",
    port=5000,
    config=CONFIG_DIR,
    debug=False,
    wsgi_profile=False,
    hookup=False,
    catch_internal_server=False
)

While all but the first two parameters are keyword arguments, the code
would actually crash if some of the keyword arguments are not provided.

In reality, there is no reason to force people to specify any parameters
whatsoever. One should simply be able to call run_api().
This commit accomplishes this by defining the appropriate default
values (without introducing breaking changes).

@codecov
Copy link

codecov bot commented Mar 28, 2020

Codecov Report

Merging #3875 into develop will decrease coverage by 0.00%.
The diff coverage is 85.71%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3875      +/-   ##
===========================================
- Coverage    77.18%   77.17%   -0.01%     
===========================================
  Files          457      457              
  Lines        33779    33775       -4     
===========================================
- Hits         26072    26067       -5     
- Misses        7707     7708       +1     
Flag Coverage Δ
#django 69.21% <85.71%> (-0.01%) ⬇️
#sqlalchemy 70.03% <85.71%> (-0.01%) ⬇️
Impacted Files Coverage Δ
aiida/restapi/resources.py 83.65% <0.00%> (ø)
aiida/restapi/run_api.py 91.42% <90.47%> (-2.52%) ⬇️
aiida/cmdline/commands/cmd_restapi.py 100.00% <100.00%> (ø)
aiida/restapi/common/config.py 100.00% <100.00%> (ø)

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 58bace3...e8067f5. Read the comment docs.

@ltalirz ltalirz requested a review from sphuber March 28, 2020 12:42
.ci/workchains.py Outdated Show resolved Hide resolved
Comment on lines 52 to 57
extensions = ['sphinx.ext.intersphinx', 'sphinx.ext.autodoc', 'sphinx.ext.doctest', 'sphinx.ext.todo',
'sphinx.ext.coverage', 'sphinx.ext.imgmath', 'sphinx.ext.ifconfig', 'sphinx.ext.viewcode',
'IPython.sphinxext.ipython_console_highlighting', 'IPython.sphinxext.ipython_directive',
'sphinxcontrib.contentui', 'aiida.sphinxext']
ipython_mplbackend = ''
Copy link
Contributor

Choose a reason for hiding this comment

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

I see some of the changes that I think you wanted to address in #3876 have snuck in. Maybe it is best to remove these here and only apply this in #3876

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, not quite - I added some new references to flask, which then caused the docs build to fail, which then caused me to add an intersphinx pointer to the flask docs here.
Which then caused me to think about adding also the docs of the python standard library (which is #3876)

Copy link
Contributor

Choose a reason for hiding this comment

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

But then don't we just want to first merge #3876 which should be fine without this PR and then you rebase this PR on top of that, which then only includes relevant changes, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

well, it's just extra effort and I think it's ok to keep the flask intersphinx mapping in this PR - otherwise I need to add another nitpick here and remove it again.
if you want me to do this, let me know.

Copy link
Contributor

Choose a reason for hiding this comment

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

I played around a bit with the intersphinx and nitpicks, and rebased your other PR and merged it. This PR is now also rebased and you already addressed my comments, so will also merge this. Thanks a lot!

@ltalirz ltalirz requested a review from sphuber March 28, 2020 19:16
The current `run_api` interface for running the AiiDA REST API had
unnecessarily many parameters, making it complicated to use in WSGI
scripts, e.g.:

```python
from aiida.restapi import api
from aiida.restapi.run_api import run_api
import aiida.restapi

CONFIG_DIR = os.path.join(os.path.split(
                  os.path.abspath(aiida.restapi.__file__))[0], 'common')

(app, api) = run_api(
    api.App,
    api.AiidaApi,
    hostname="localhost",
    port=5000,
    config=CONFIG_DIR,
    debug=False,
    wsgi_profile=False,
    hookup=False,
    catch_internal_server=False
)
```
While all but the first two parameters are keyword arguments, the code
would actually crash if they are not provided.

In reality, there is no reason to have to specify *any* parameters
whatsoever and one should simply be able to call `run_api()`.
This commit accomplishes this by defining the appropriate default
values.
@sphuber sphuber force-pushed the issue_3870_rest_api_interface branch from 59d76ca to e8067f5 Compare March 29, 2020 20:51
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks a lot @ltalirz

@sphuber sphuber merged commit 0f069a7 into aiidateam:develop Mar 29, 2020
@sphuber sphuber deleted the issue_3870_rest_api_interface branch March 29, 2020 21:10
@ltalirz
Copy link
Member Author

ltalirz commented Mar 29, 2020 via email

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.

run_api interface is cumbersome and error-prone
2 participants