Expose AmountTaxScale by Web API

[sandcha]

New features

• Introduce scales with amount keys in Web API
  • Allows for AmountTaxScale parameters use through Web API.
    Example: See this parameter in openfisca-tunisia.

Note:

Scales with amount key already exist in Core but for the moment, the Web API exposes only one type of scale: scales with rate key.
This missing type generates an error on openfisca-tunisia Web API and this PR comes to fix it.

Details:

Calling the following command:

openfisca serve --country-package openfisca_tunisia

With:

OpenFisca-Core    24.6.4     
OpenFisca-Tunisia 0.21.0 

Leads to the following error:

[2018-11-04 19:48:49 +0100] [21181] [ERROR] Exception in worker process
Traceback (most recent call last):

...

  File ".../virtualenvs/tnc24/lib/python3.7/site-packages/openfisca_web_api/loader/parameters.py", line 83, in walk_node
    api_parameter['brackets'] = build_api_scale(child)
  File ".../virtualenvs/tnc24/lib/python3.7/site-packages/openfisca_web_api/loader/parameters.py", line 33, in build_api_scale
    } for bracket in scale.brackets]
  File ".../virtualenvs/tnc24/lib/python3.7/site-packages/openfisca_web_api/loader/parameters.py", line 33, in <listcomp>
    } for bracket in scale.brackets]
AttributeError: 'Bracket' object has no attribute 'rate'
[2018-11-04 19:48:49 +0100] [21180] [INFO] Worker exiting (pid: 21180)
[2018-11-04 19:48:50 +0100] [21177] [INFO] Shutting down: Master
[2018-11-04 19:48:50 +0100] [21177] [INFO] Reason: Worker failed to boot.

[benjello]

@sandcha : I am all fot ir but I am not sure I am the more able to review it ...

[sandcha]

Thx @benjello! If you reproduce the error for openfisca serve --country-package openfisca_tunisia on master branch and see that the branch of this PR resolves it, we can already say that we have a better situation.

The API really works when the following command response is a welcome to the Web API:
curl http://localhost:5000
And the response structure for a parameter with amount shouldn't be missing any content of the parameter yaml file. You can check it for:
curl http://localhost:5000/parameter/impot_revenu/contribution_budget_etat

Besides, the structure of the response should be similar to a response for a scale parameter with rate items; at least, imho, the unit brings enough information to distinguish between rates and amounts. Tell me if you agree with this (you can get all parameters paths with http://localhost:5000/parameters and check any rates).

It could also be checked it with another country model to confirm that it doesn't come with new bugs. ^^

[benjello]

@sandcha : I agree that the unit brings the right information.

[Morendil]

After reviewing this IRL, some remarks:

• the need for this fix arises because the internal representation of scale parameters and the API's representation of them are wildly divergent;
• the fix slightly degrades the quality and readability of the code that translates between representations, but it's probably not worth trying to do better;
• rather, bringing the internal representation in line with what the API exposes seems like a more promising investment.

[bonjourmauko]

Tests?

[bonjourmauko]

The need for this fix arises because the internal representation of scale parameters and the API's representation of them are wildly divergent;

Bringing the internal representation in line with what the API exposes seems like a more promising investment

A more REST approach would be to properly separate internal and external representations... Wouldn't a RateSerializer and an AmountSerializer with polymorphic dispatch solve de issue quite simply?

[Morendil]

Tests?

"We can start openfisca serve --country-package openfisca_tunisia without the server crashing" has served as a smoke test. We are much hampered from doing anything smaller-grained by the need to add specific types of rates to the country template (with attendant PR and review cycle), as opposed to fixturing them in locally.

[Morendil]

Wouldn't a RateSerializer and an AmountSerializer with polymorphic dispatch solve de issue quite simply?

Unfortunately no, due to substantial (and, to @sandcha and I, unsuspected) technical debt in that area (I think I'm using the term advisedly here, as opposed to "cruft", since @sandcha informs me this was a conscious decision to rapidly ship substantial changes in the API's external representation).

There are two layers of objects to be concerned with:

• Parameters, Scales and ParameterNodes form a quasi-Composite pattern; the "quasi" here denotes the fact that instead of the usual subclass relationship between Composite and Component, there is instead a duck-typing of the shared protocol, which consists of the get_at_instant call (more commonly mediated by a function-like form, as parameter(period)); Parameter's get_at_instant directly returns a scalar value, and this is represented easily in the API;
• however, Scales encapsulate a distinct class hierarchy which doesn't offer that protocol; you cannot directly call get_at_instant on an AmountTaxScale or any AbstractTaxScale subclass; rather when you call get_at_instant on a Scale it instantiates an AbstractTaxScale subclass on the fly with a collection of Brackets.

Why?

• this is because the internal representation of these scales is actually inconsistent with the semantics exposed externally by the API; instead of several copies of these scales at various points in time, what we have internally is a series of (for instance) amounts and thresholds that can each vary independently over time, there are therefore more possibilities representable internally than externally;
• because of that, the representation of these scales "for all periods" simply cannot be had by dispatching to these classes, hence the ugly isinstance(child, Scale) which is being extended in this PR.

[bonjourmauko]

We are much hampered from doing anything smaller-grained by the need to add specific types of rates to the country template (with attendant PR and review cycle), as opposed to fixturing them in locally.

It seems like we're tampering our own rules. If the way we're testing the API now is a constraint, what can we do to exploit, subordinate or elevate it? Isn't fixturing/mocking for this specific case better than no testing at all? Isn't this too urgent that we cannot wait for the PR and review cycle?

"We can start openfisca serve --country-package openfisca_tunisia without the server crashing" has served as a smoke test.

Tests must be automated, predictable and idempotent. If by adding openfisca serve --country-package openfisca_tunisia to CircleCI we can achieve that, then let's go for it.

[Morendil]

Isn't fixturing/mocking for this specific case better than no testing at all?

Let's see what we can do. @sandcha I'll be adding commits / rebasing.

[Morendil]

@maukoquiroga New test in tests/web_api/test_scale_serialization.py.

[bonjourmauko]

Which doesn't offer that protocol

Fair enough. If they don't share the same protocol, those objects become hard to dispatch.

The representation of these scales "for all periods" simply cannot be had by dispatching to these classes, hence the ugly isinstance(child, Scale) which is being extended in this PR

I bit beyond the scope of my initial comment, but I agree in Python type checking has it's good use for static analysis, not for control flow.

I modified this pull request myself, so I'm dismissing my review.

[bonjourmauko]

@Morendil Thanks for the tests, I added minimal changes to make them unit tests.

@sandcha @fpagnoux @pblayo What do you think?