Create a simple computation endpoint #528

fpagnoux · 2017-06-21T09:51:09Z

Connected to #520

Sorry, I'm opening this PR late.

The spec has not yet been updated. If anyone has time to do it, that would be very appreciated.

Anna-Livia

Thank you for all of this work !

Anna-Livia · 2017-06-21T12:45:19Z

openfisca_core/simulations.py

@@ -151,7 +198,7 @@ def get_array(self, column_name, period):
            caller_input_variables_infos = calling_frame['input_variables_infos']
            if variable_infos not in caller_input_variables_infos:
                caller_input_variables_infos.append(variable_infos)
-        return self.get_or_new_holder(column_name).get_array(period)
+        return self.get_variable_entity(column_name).get_array(period).get_holder(column_name).get_array(period)


Je ne comprends pas très bien cet enchainement d'objets. C'est l'array qui est dans le holder de l'array de la variable ?

👍 Il y a clairement un get_array de trop.

Et ce n'était pas la seule erreur de regexp ^^

fpagnoux · 2017-06-21T20:06:14Z

If the response body doesn't show in case of error in Swagger UI, it's not our fault.

Upgrading Swagger UI may partially fix the problem in the legislation explorer.

fpagnoux · 2017-06-21T20:07:22Z

To be done:

Complete the JSON schema
~~Refactor YAML spec (headers are defined n times)~~

Headers cannot be defined once for several responses:

Note that, currently, there is no way in Swagger to define common response headers for different response codes or different API operations. You need to define the headers for each response individually.

sandcha · 2017-06-21T14:07:57Z

CHANGELOG.md

+- Deprecate `simulation.holder_by_name`, `simulation.get_holder`, `get_or_new_holder`
+  - These functionalities are now provided by `entity.get_holder(name)`
+
+- Deprecate constructor `Holder(simulation, column`


Typo: missing )

sandcha · 2017-06-21T14:57:05Z

tests/web_api/test_calculate.py

+        ('{"a" : "x", "b"}', BAD_REQUEST, 'error', 'Invalid JSON'),
+        ('["An", "array"]', BAD_REQUEST, 'error', 'Invalid type'),
+        ('{"persons": {}}', BAD_REQUEST, 'persons', 'At least one person'),
+        ('{"persons": {"bob": {}}, "unknown_entity": {}}', BAD_REQUEST, 'unknown_entity', 'entity is not defined',),


BAD_REQUEST or NOT_FOUND ? What's the difference between an unknown entity and an unknown variable for a user ? Both come from openfisca world. NOT_FOUND for both ?

During the workshop on the API, we agreed to return a 400 in the case of an unexpected entity.

The main difference is that:

Unexpected entities rarely happen

Unexpected variables happen more often, for instance when the user requests a deprecated or renamed variable

We just want a specific error code for the last case, so that we can easily detect it when it happens.

sandcha · 2017-06-21T16:55:12Z

CHANGELOG.md

+  - For instance `Simulation(simulation_json = {"persons": {...}, "households": {...}}, tax_benefit_system = tax_benefit_system)`
+
+- Allows entities to be built from a JSON using their constructor
+  - For instance `Household(simulation, {"first_household": {...}})`


Add entities_json = {"first_household": {...}} ?

A keyword argument is not needed in this case, as entities_json is the second argument.

The idea is:

There was a Household(simulation) constructor

Now there is also a Household(simulation, entities_json) constructor

sandcha · 2017-06-21T17:02:20Z

openfisca_core/simulations.py

+    type_map = {
+        dict: "Object",
+        list: "Array",
+        str: "String"


What kind of types are they ? Json types ? Add the information in function name or in documentation ?

sandcha · 2017-06-21T22:48:51Z

openfisca_core/entities.py

+            except VariableNotFound as e:  # The variable doesn't exist
+                raise SituationParsingError([self.plural, entity_id, variable_name], e.message, code = 404)
+
+            holder = self.get_holder(variable_name)


Move holder init after if condition ?

sandcha · 2017-06-21T23:51:01Z

openfisca_core/entities.py

+            for person_id in role_definition:
+                if person_id not in self.simulation.persons.ids:
+                    raise SituationParsingError([self.plural, entity_id, role_id],
+                        "Unexpected value: {0}. {0} has been declared in {1} {2}, but has not been declared in {3}.".format(


sandcha · 2017-06-21T23:54:11Z

openfisca_core/entities.py


    def check_role_validity(self, role):
        if role is not None and not type(role) == Role:
-            raise Exception("{} is not a valid role".format(role))
+            raise ValueError("{} is not a valid role".format(role))


Catch the exception where check_role_validity is called ?

check_role_validity is not called in the entity initialization process.
This was just an opportunistic fix.

sandcha · 2017-06-21T23:59:33Z

openfisca_core/holders.py

+                u"The Holder(simulation, column) constructor has been deprecated. "
+                u"Please use Holder(entity = entity, column = column) instead.",
+                Warning
+                )


sandcha · 2017-06-22T00:00:23Z

openfisca_core/holders.py

@@ -255,12 +266,19 @@ def put_in_cache(self, value, period, extra_params = None):
                raise ValueError('A period must be specified to put values in cache, except for variables with ETERNITY as as period_definition.')
            if ((self.column.definition_period == MONTH and period.unit != periods.MONTH) or
               (self.column.definition_period == YEAR and period.unit != periods.YEAR)):
-                raise ValueError(u'Unable to set a value for variable {0} for {1}-long period {2}. {0} is only defined for {3}s. Please adapt your input. If you are the maintainer of {0}, you can consider adding it a set_input attribute to enable automatic period casting.'.format(
+                error_message = u'Unable to set a value for variable {0} for {1}-long period {2}. {0} is only defined for {3}s. Please adapt your input. If you are the maintainer of {0}, you can consider adding it a set_input attribute to enable automatic period casting.'.format(


Add new lines in message ?

sandcha · 2017-06-22T00:07:05Z

openfisca_core/simulations.py

+                entities = entity_class(self, entities_json)
+            else:
+                entities = entity_class(self)
+            self.entities[entity_class.key] = entities


Add entities = None ... at Simulation class first lines ?

Normally, what you declare on the first lines of a class are class attributes (similar to static attributes in Java).

Using class attributes to define instances attributes default values technically works (only because the default attributes are not mutable), but it's not a good practise.

I know, we do this all over the codebase 😞

sandcha · 2017-06-22T09:11:41Z

CHANGELOG.md

+- Handle `500` errors in the preview API
+    - In this case, the API returns a JSON with details about the error.
+
+- Allows simulations to be build from a JSON using their constructor


Typo: builT

sandcha · 2017-06-22T09:15:21Z

CHANGELOG.md

+#### New features
+
+- Introduce `/calculate` route in the preview API
+  - Allows to run calculations


Explain briefly that it handles calculations with json requests format ?

sandcha · 2017-06-22T09:31:05Z

openfisca_core/entities.py

@@ -89,6 +194,22 @@ def filled_array(self, value, dtype = None):
            warnings.simplefilter("ignore")
            return np.full(self.count, value, dtype)

+    def get_holder(self, variable_name, init = True):


def holder_value(self, variable_name, init = True) ? Or holder_property.

fpagnoux force-pushed the calculate-route branch from d781d49 to 370825d Compare June 21, 2017 09:51

fpagnoux requested review from MattiSG, sandcha, Anna-Livia, michelbl and cbenz June 21, 2017 09:52

Anna-Livia reviewed Jun 21, 2017

View reviewed changes

fpagnoux mentioned this pull request Jun 21, 2017

Upgrade swagger to display error responses body openfisca/legislation-explorer#97

Merged

fpagnoux self-assigned this Jun 21, 2017

sandcha requested changes Jun 22, 2017

View reviewed changes

fpagnoux requested a review from sandcha June 22, 2017 08:03

sandcha requested changes Jun 22, 2017

View reviewed changes

sandcha approved these changes Jun 22, 2017

View reviewed changes

fpagnoux force-pushed the calculate-route branch 2 times, most recently from 5523d61 to 35ca745 Compare June 22, 2017 10:52

fpagnoux added 13 commits June 22, 2017 12:52

Handle some bad request cases

a97353d

Improve entities constructor

4955be0

Refactor

70e6750

Refactor test

8155b52

Init situation with persons variable

71dc291

Test set_inputs

e012e37

Handle basic calculation

5a98c0e

Ignore null values when parsing situation

3bef06b

Minor changes: flake8

c4546db

Refactor entities

7a373bc

Pass core tests

aedddeb

Do not use deprecated function

9b2394a

Handle more error cases

c35e597

fpagnoux added 26 commits June 22, 2017 12:52

Handle period mismatch

dab6404

Change breaking changes to deprecations

60e9d1f

Move handle_invalid_json definition

66b9f3e

Minor refactor

7d8e5f1

Handle set_input errors

e3a4a90

Add dpath to dependencies

be9a020

Detect invalid periods for requested variables

3ad1c05

Refactor simulation.instantiate_entities

7b98907

Ignore unexpected nulls

c725b2c

Refactor /calculate

e741d2a

Bump version number

f1be17f

Upgrade country template

4f105c9

Document /calculate route

05ac86b

Refactor OpenAPI spec generation

22262e5

Document variable values type in Schema

0f24871

Improve OpenAPI spec

6623600

Remove spaces before ":"

080ecb4

Rename build_from_json - > init_from_json

54c72bb

Minor refactor

d9979c9

Minor refactor

ac7c7e9

Minor refactor

38a8cf5

Handle ETERNITY in input

cdbb771

Handle unique roles

d80924c

Handle invalid type in role array

48f197f

Minor refactor

2096537

Minor refactor

659de24

fpagnoux force-pushed the calculate-route branch from 35ca745 to 659de24 Compare June 22, 2017 10:53

fpagnoux merged commit d9ad655 into master Jun 22, 2017

bonjourmauko deleted the calculate-route branch April 10, 2018 18:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a simple computation endpoint #528

Create a simple computation endpoint #528

fpagnoux commented Jun 21, 2017 •

edited

Loading

Anna-Livia left a comment

Anna-Livia Jun 21, 2017

fpagnoux Jun 21, 2017

fpagnoux Jun 21, 2017

fpagnoux commented Jun 21, 2017 •

edited

Loading

fpagnoux commented Jun 21, 2017 •

edited

Loading

sandcha Jun 21, 2017

fpagnoux Jun 22, 2017

sandcha Jun 21, 2017

fpagnoux Jun 22, 2017 •

edited

Loading

sandcha Jun 21, 2017

fpagnoux Jun 22, 2017 •

edited

Loading

sandcha Jun 21, 2017

fpagnoux Jun 22, 2017

sandcha Jun 21, 2017

fpagnoux Jun 22, 2017

sandcha Jun 21, 2017

sandcha Jun 21, 2017

fpagnoux Jun 22, 2017

sandcha Jun 21, 2017

sandcha Jun 22, 2017

fpagnoux Jun 22, 2017

sandcha Jun 22, 2017

fpagnoux Jun 22, 2017

sandcha Jun 22, 2017

sandcha Jun 22, 2017

sandcha Jun 22, 2017

Create a simple computation endpoint #528

Create a simple computation endpoint #528

Conversation

fpagnoux commented Jun 21, 2017 • edited Loading

Anna-Livia left a 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 Jun 21, 2017 • edited Loading

fpagnoux commented Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpagnoux Jun 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpagnoux Jun 22, 2017 • 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

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 Jun 21, 2017 •

edited

Loading

fpagnoux commented Jun 21, 2017 •

edited

Loading

fpagnoux commented Jun 21, 2017 •

edited

Loading

fpagnoux Jun 22, 2017 •

edited

Loading

fpagnoux Jun 22, 2017 •

edited

Loading