Make Web API optional #703

fpagnoux · 2018-08-02T02:41:49Z

Depends on #702
Fixes #641
Fixes #592
Fixes #665
Fixes #663 (by integrating @MattiSG's changes from the openapi branch)

Breaking changes

Only install the Web API dependencies as an opt-in:

pip install OpenFisca-Core will not install the Web API anymore.
pip install OpenFisca-Core[api] will.

Country package maintainers who still want to provide the Web API by default with their package (recommended) should update their setup.py:

In the install_requires section, replace 'OpenFisca-Core >= 23.3, < 24.0' by 'OpenFisca-Core[api] >= 24.0, < 25.0'
See example

~~Country package maintainers who want to provide the Web API as an opt-in of their package should update their setup.py:~~

~~In the extras_require section, add an api block containing ['OpenFisca-Core[api]']~~
~~See example~~

NOTE: This doesn't work, there is no way to "forward" the extra dependency from france to core

Change default Web API port to 5000:

openfisca serve will now serve by default on the 5000 port instead of 6000 (blocked by Chrome).

Rename OpenFisca Web Api package to `openfisca_web_api`:

Transparent for users of the openfisca serve command.
Users who used to manually import openfisca_web_api_preview must know import openfisca_web_api.

New features

In the /spec route:
- Indicate the country package version instead of 0.1.0.
- Infer the host URL from the requests, instead of relying on the undocumented SERVER_NAME environnement variable.
  - The use of the SERVER_NAME environnement variable is therefore deprecated and without effect.

fpagnoux · 2018-08-02T04:12:06Z

Country package maintainers who want to provide the Web API as an opt-in of their package should update their setup.py:

In the extras_require section, add an api block containing ['OpenFisca-Core[api]']

See example

This actually doesn't work, because... of pip! Ha ha, who could have guessed?

Looking to find a better solution tomorrow, but that's not blocking for the review, as there is not much we can do on this repo about that.

fpagnoux · 2018-08-02T04:12:54Z

Error message when running openfisca serve without having installed the extra dep:

(no-api) ➜  openfisca-france git:(optional-api) ✗ openfisca serve
Traceback (most recent call last):
  File "/Users/florianpagnoux/.virtualenvs/noa/lib/python3.7/site-packages/openfisca_web_api/app.py", line 18, in <module>
    from flask import Flask, jsonify, abort, request, make_response
ModuleNotFoundError: No module named 'flask'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/florianpagnoux/.virtualenvs/noa/bin/openfisca", line 7, in <module>
    from openfisca_core.scripts.openfisca_command import main
  File "/Users/florianpagnoux/.virtualenvs/noa/lib/python3.7/site-packages/openfisca_core/scripts/openfisca_command.py", line 5, in <module>
    from openfisca_web_api.scripts.serve import define_command_line_options, main as serve
  File "/Users/florianpagnoux/.virtualenvs/noa/lib/python3.7/site-packages/openfisca_web_api/scripts/serve.py", line 9, in <module>
    from openfisca_web_api.app import create_app
  File "/Users/florianpagnoux/.virtualenvs/noa/lib/python3.7/site-packages/openfisca_web_api/app.py", line 22, in <module>
    handle_import_error(error)
  File "/Users/florianpagnoux/.virtualenvs/noa/lib/python3.7/site-packages/openfisca_web_api/errors.py", line 10, in handle_import_error
    raise ImportError("OpenFisca is missing some dependencies to run the Web API: '{}'. To install them, run `pip install openfisca_core[api]`.".format(error))
ImportError: OpenFisca is missing some dependencies to run the Web API: 'No module named 'flask''. To install them, run `pip install openfisca_core[api]`.

sandcha · 2018-08-02T14:56:28Z

CHANGELOG.md

+- Transparent for users of the `openfisca serve` command.
+- Users who used to manually import `openfisca_web_api_preview` must know import `openfisca_web_api`.
+
+##### Rename development dependencies to `dev`:


Make test dependencies tag update more discoverable? : ##### Rename test dependencies to dev

sandcha · 2018-08-02T14:59:45Z

README.md

@@ -6,7 +6,9 @@ This package contains the core features of OpenFisca, which are meant to be used

 ## Environment

-This package requires Python 2.7
+OpenFisca runs on Python 3.6, or more recent versions.


3.6 🙌
And remove , or more recent versions according to recent issues when moving between 3.* versions

sandcha · 2018-08-03T09:04:03Z

README.md

@@ -17,7 +19,7 @@ If you want to contribute to OpenFisca-Core itself, welcome! To install it local
 ```bash
 git clone https://github.com/openfisca/openfisca-core.git
 cd openfisca-core
-pip install --editable ".[test]"
+pip install --editable ".[api, dev]"


This seems to say that the API comes from api tag while it also comes with dev that installs openfisca-country-template (as openfisca-country-template comes with 'OpenFisca-Core[api] >= 24.0, < 25.0', in its next version). 😕

I don't know exactly what's better; make a new tag in country-template that depends on Core without api and call it in dev?

This seems to say that the API comes from api tag while it also comes with dev that installs openfisca-country-template (as openfisca-country-template comes with 'OpenFisca-Core[api] >= 24.0, < 25.0', in its next version). 😕

Does this cause any concrete issue?

The way pip probably sees it is:

Install the extra dependency api

Install the extra dependency dev

Install OpenFisca-Country-Template

This requires OpenFisca-Core[api], but this is already installed, so doing nothing.

I'm not comfortable with this solution for multiple reasons :

api isn't really useful here

it might be confusing for a user that removes api from pip install --editable ".[api, dev]" to find api dependencies installed

we are changing pip install -e . definition for everyone

To imitate an opt-out @fpagnoux, what do you think of the following code for setup.py? :

import os api_requirements = [ 'flask == 0.12', 'flask-cors == 3.0.2', 'gunicorn >= 19.7.1', ] OPENFISCA_API = os.environ.get('OPENFISCA_API') if OPENFISCA_API == "no": api_requirements = []

and :

install_requires = [ 'Biryani[datetimeconv] >= 0.10.8', 'dpath == 1.4.0', 'enum34 >= 1.1.6', 'future', 'numpy >= 1.11, < 1.15', 'psutil == 5.4.6', 'PyYAML >= 3.10', 'sortedcontainers == 1.5.9', api_requirements ],

where OPENFISCA_API environment variable would only be needed for opt-out.

Hmm interesting, I didn't think about tweaking the setup.py that way.

Let me check if that really works, but if it does I'm happy to switch to this.

Unfortunately, while tweaking the setup.py is definitely a great idea (and will solve Anna-Livia's concern) , the environment variable doesn't work in real conditions.

If I run OPT_OUT_API=true pip install -e ., everything works as expected and the API dependencies are not installed.

However, if I run:
OPT_OUT_API=true pip install 'OpenFisca-Core >= 24.0.0.dev2, < 25', the environment variable is ignored, and the API dependencies are installed.

I think the setup.py file is executed only:

when installing Core locally with pip install .

when publishing with python setup.py bdist_wheel

Ok, I included api dependencies into dev, which should address your two first concerns, as well as @Anna-Livia's.

However, yes:

we are changing pip install -e . definition for everyone

but, as of today, we didn't find a way to implement an "opt-out" dependency. We reached this conclusion in June, and while your suggestion was definitely welcome and worth trying, it didn't work.

The only approach I could see to keep the API included by default would be to publish alternative packages OpenFisca-Core-Lite and OpenFisca-France-Lite that don't include the API, but that's really heavy.

So, unless someone has a better solution, making the API installation an opt-in, while opting in by default in the country template seems the most reasonable thing to do if we want to make it possible to install OpenFisca without the API

Thanks for these tests. Would it be ok for you to give us 24 more hours to figure something out? With @Anna-Livia, we are looking into pip options.

Ok 🙂. Keep me posted if you find something!

sandcha · 2018-08-03T13:31:25Z

openfisca_core/scripts/find_placeholders.py

@@ -1,5 +1,7 @@
 # -*- coding: utf-8 -*-

+from __future__ import print_function
+from __future__ import unicode_literals


sandcha · 2018-08-03T13:36:47Z

openfisca_core/scripts/simulation_generator.py

@@ -1,3 +1,4 @@
+from __future__ import unicode_literals


Unused import?

This part comes from #702, which this PR depends on.

For consistency, we chose to import all __future__ backports in all files.

sandcha · 2018-08-03T13:46:31Z

openfisca_core/variables.py

@@ -358,6 +358,9 @@ def get_introspection_data(cls, tax_benefit_system):
            source_file_path = absolute_file_path.replace(tax_benefit_system.get_package_metadata()['location'], '')
        try:
            source_lines, start_line_number = inspect.getsourcelines(cls)
+            # Python 2 backward compatibility


Discussed here

sandcha · 2018-08-03T14:24:29Z

openfisca_web_api/loader/spec.py

@@ -16,7 +16,7 @@ def build_openAPI_specification(tax_benefit_system, country_package_metadata):
    country_package_name = country_package_metadata['name'].title()
    spec['info']['title'] = spec['info']['title'].replace("{COUNTRY_PACKAGE_NAME}", country_package_name)
    spec['info']['description'] = spec['info']['description'].replace("{COUNTRY_PACKAGE_NAME}", country_package_name)
-    spec['host'] = os.environ.get('SERVER_NAME')
+    spec['info']['version'] = country_package_metadata['version']


sandcha · 2018-08-03T14:29:10Z

setup.py

            'nose',
            'flake8 >= 3.4.0, < 3.5.0',
-            'openfisca-country-template >= 3.2.2rc0, < 4.0.0',
+            'openfisca-country-template >= 3.2.3, < 4.0.0',


>= 3.3.0, 4.0.0?

This is not necessary to pass the tests, and I'd rather not do it to avoid the complexity of releasing inter-dependent packages with rc0 versions.

sandcha · 2018-08-03T14:34:30Z

tests/core/test_scenarios.py

@@ -1,3 +1,4 @@
+from __future__ import unicode_literals


Unused import?

This part comes from #702, which this PR depends on.

For consistency, we chose to import all __future__ backports in all files.

sandcha · 2018-08-03T15:00:29Z

CHANGELOG.md

+#### New features
+
+- In the `/spec` route:
+  - Indicate the country package version instead of `0.1.0`.


Indicate served country package version as API version?

fpagnoux · 2018-08-03T17:21:07Z

Sorry, it was probably not super clear that part of the diff is from another PR.

Anna-Livia · 2018-08-06T12:42:07Z

It was very strange as a developper to have make test fail after running pip install openfisca-core .[dev] because I hadn't installed the api first.
It feels that pip install openfisca-core .[dev] should install the API. WDYT ?

Anna-Livia · 2018-08-08T16:11:19Z

So, I looked into the Env Vars. The reason it works with pip install . and not with pip install openfisca-core is because . is a python project, and openfisca-core is a wheel.
When you pip install a python project, os.environ in setup.py is the user's environment.
When you pip install a wheel, os.environin setup.py is the wheel builder's environment.
The wheels figure out all the dependencies when it's built.

There is a way to pass env vars, but very specific sys variables to have conditionnal dependencies such as python version, or os.

Anywho, in my mind, we have two options :
A/ Change to openfisca-core[web] to install the web-api dependencies. (Web is more meaningful in my mind than API, because we also have a python api)
B/ have a NO_API env var, to be used with pip install .

Looking forward to have your opinion on this :)

fpagnoux · 2018-08-08T18:21:32Z

B/ have a NO_API env var, to be used with pip install .

I don't think this makes sense for the people who asked to make the Web API install optional. Economists who install OpenFisca in a secure environment don't want to install Core in editable mode. That would actually prevent them from relying on versioning.

A/ Change to openfisca-core[web] to install the web-api dependencies. (Web is more meaningful in my mind than API, because we also have a python api)

I think api should be in the dependency title. What about making it fully explicit: web-api (or web_api)?

Anna-Livia · 2018-08-10T09:47:14Z

.circleci/config.yml

@@ -22,7 +22,7 @@ jobs:
          name: Install dependencies
          command: |
            pip install --upgrade pip twine wheel
-            pip install --editable .[test] --upgrade
+            pip install --editable ".[api, dev]" --upgrade


With the lastest changes, shouldn't it be pip install --editable ".[dev]" --upgrade ?

Good catch, forgot that one!

Anna-Livia · 2018-08-10T09:52:36Z

README.md


-To run it with the mock country package `openfisca_country_template` and another port value as `5000`, run:
+To run it with the mock country package `openfisca_country_template` and another port value such as `2000`, run:


I feel we should mention that openfisca_country_template should be installed with the api extra requirements.

proposition :

To serve the Web-API, you need to install a country package with the [web_api] extra require.
e.g.

pip install openfisca-country-template[web_api]

To run it with the mock country package openfisca_country_template and another port value such as 2000, run:
[...]

Actually, in the country template PR there is no web_api extra dependencies and it is required by default.

Also it is not possible to install the web api with something like pip install Openfisca-France[web-api] that would require Openfisca-Core[web-api].

🌴🏖🌴

And rename test -> dev

fpagnoux requested review from sandcha, Anna-Livia and bonjourmauko August 2, 2018 02:43

This was referenced Aug 2, 2018

Adapt to Core v24 openfisca/country-template#51

Merged

Rend l'installation de la Web API optionnelle openfisca/openfisca-france#1062

Merged

fpagnoux changed the title ~~Make Web API optionnal~~ Make Web API optional Aug 2, 2018

fpagnoux force-pushed the optional-web-api branch from e7df364 to 37591a4 Compare August 2, 2018 04:38

sandcha previously requested changes Aug 3, 2018

View reviewed changes

fpagnoux requested a review from sandcha August 3, 2018 17:20

Anna-Livia assigned sandcha Aug 6, 2018

fpagnoux requested review from sandcha and removed request for sandcha August 6, 2018 22:50

Anna-Livia approved these changes Aug 10, 2018

View reviewed changes

fpagnoux force-pushed the optional-web-api branch 3 times, most recently from 7db48a0 to 49ff624 Compare August 15, 2018 15:17

fpagnoux and others added 7 commits August 15, 2018 11:23

Make api dependencies optionnal

315bfbd

Switch openfisca.org URLs to HTTPS

50ffaca

Remove unnecessary HTML extension

64c4c36

Consider Core-provided Web API as stable

7c447e1

Install api dep in doc & circle

20e906b

And rename test -> dev

Bump version number

5637240

Change default port to 5000

32fdc18

fpagnoux added 4 commits August 15, 2018 11:35

State actual version in the spec

a1e7845

Infer host name from requests

548db39

Update changelog

d742064

Remove opt-in suggestion that pip can't handle

0874c5c

fpagnoux force-pushed the optional-web-api branch from 49ff624 to 7e39a95 Compare August 15, 2018 15:35

fpagnoux added 4 commits August 15, 2018 11:37

Strongly recommend Python 3.6

912c56c

Use env var for opt out

e472d7e

Include api dep in dev dep

31871c8

Rename extra dep to "web-api"

b8c3a4d

fpagnoux force-pushed the optional-web-api branch from 7e39a95 to b8c3a4d Compare August 15, 2018 15:37

fpagnoux merged commit b296504 into master Aug 15, 2018

fpagnoux deleted the optional-web-api branch August 15, 2018 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Web API optional #703

Make Web API optional #703

fpagnoux commented Aug 2, 2018 •

edited

Loading

fpagnoux commented Aug 2, 2018

fpagnoux commented Aug 2, 2018 •

edited

Loading

sandcha Aug 2, 2018

sandcha Aug 2, 2018

sandcha Aug 3, 2018

fpagnoux Aug 3, 2018

sandcha Aug 6, 2018 •

edited

Loading

fpagnoux Aug 6, 2018

fpagnoux Aug 6, 2018

fpagnoux Aug 6, 2018

sandcha Aug 7, 2018

fpagnoux Aug 7, 2018

sandcha Aug 3, 2018

sandcha Aug 3, 2018

fpagnoux Aug 3, 2018

sandcha Aug 3, 2018

fpagnoux Aug 3, 2018 •

edited

Loading

sandcha Aug 3, 2018

sandcha Aug 3, 2018

fpagnoux Aug 3, 2018

sandcha Aug 3, 2018

fpagnoux Aug 3, 2018

sandcha Aug 3, 2018

fpagnoux commented Aug 3, 2018

Anna-Livia commented Aug 6, 2018

Anna-Livia commented Aug 8, 2018

fpagnoux commented Aug 8, 2018 •

edited

Loading

Anna-Livia Aug 10, 2018

fpagnoux Aug 10, 2018

Anna-Livia Aug 10, 2018

fpagnoux Aug 10, 2018


		To run it with the mock country package `openfisca_country_template` and another port value as `5000`, run:
		To run it with the mock country package `openfisca_country_template` and another port value such as `2000`, run:

Make Web API optional #703

Make Web API optional #703

Conversation

fpagnoux commented Aug 2, 2018 • edited Loading

Breaking changes

Only install the Web API dependencies as an opt-in:

Change default Web API port to 5000:

Rename OpenFisca Web Api package to openfisca_web_api:

New features

fpagnoux commented Aug 2, 2018

fpagnoux commented Aug 2, 2018 • 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

sandcha Aug 6, 2018 • 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

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

fpagnoux Aug 3, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpagnoux commented Aug 3, 2018

Anna-Livia commented Aug 6, 2018

Anna-Livia commented Aug 8, 2018

fpagnoux commented Aug 8, 2018 • 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

fpagnoux commented Aug 2, 2018 •

edited

Loading

Rename OpenFisca Web Api package to `openfisca_web_api`:

fpagnoux commented Aug 2, 2018 •

edited

Loading

sandcha Aug 6, 2018 •

edited

Loading

fpagnoux Aug 3, 2018 •

edited

Loading

fpagnoux commented Aug 8, 2018 •

edited

Loading