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

Revert 3.2.9: "Refactor tracing implementation" #898

Closed
wants to merge 2 commits into from

Conversation

bonjourmauko
Copy link
Member

@bonjourmauko bonjourmauko commented Jul 16, 2019

Bug fix

Fixes #899
Fixes openfisca/openfisca-france#1360

Note: Versions 3.2.9 and 3.3.0 have been unpublished as the
former accidentally introduced a bug affecting the Web API. Please use
version 3.3.1 or more recent.

  • Revert version 3.2.9 as it introduced a bug that rendered the Web
    API unusable
    • In this version, the tracer outputs numpy.ndarray as a valid
      value for parameters
    • Since numpy.ndarray is not JSON serializable, the Web API crashes
      when trying to serialize a trace
How to reprode the error

You can use the following snippet:

from flask import Flask, jsonify
from openfisca_core.scripts import build_tax_benefit_system
from openfisca_web_api.loader import build_data

def trace():
    tax_benefit_system = build_tax_benefit_system("openfisca_france", None, None)
    data = build_data(tax_benefit_system)
    return data["openAPI_spec"]["definitions"]["Trace"]["example"]["trace"]

def acc(data, key):
    params = data[key]["parameters"]
    return [(key, param, params[param], type(params[param])) for param in params.keys()]

def serializable(value):
    try:
        jsonify({**value})
        return True
    except TypeError:
        return False

with Flask(__name__).app_context():
    data = trace()
    result = [acc(data, key) for key, _ in data.items() if not serializable(data[key])]
Expected
result
>>> []
Actual
result[0][0]
(
  'aide_logement_loyer_plafond<2017-12>',
  'prestations.aides_logement.loyers_plafond.par_zone<2017-12-01>',
  array([51.54]),
  numpy.ndarray),
  )

> Note: Versions `3.2.9` and `3.3.0` have been unpublished as the
former accidentally introduced a bug affecting the Web API. Please use
version `3.3.1` or more recent.

- Revert version 3.2.9 as it introduced a bug that rendered the Web
API unusable
  - In this version, the tracer outputs `numpy.ndarray` as a valid
value for `parameters`
  - Since `numpy.ndarray` is not JSON serializable, the Web API crashes
when trying to serialize a trace

**How to reprode the error**

You can use the following snippet:

```python
from flask import Flask, jsonify
from openfisca_core.scripts import build_tax_benefit_system
from openfisca_web_api.loader import build_data

def trace():
    tax_benefit_system = build_tax_benefit_system("openfisca_france", None, None)
    data = build_data(tax_benefit_system)
    return data["openAPI_spec"]["definitions"]["Trace"]["example"]["trace"]

def acc(data, key):
    params = data[key]["parameters"]
    return [(key, param, params[param], type(params[param])) for param in params.keys()]

def serializable(value):
    try:
        jsonify({**value})
        return True
    except TypeError:
        return False

with Flask(__name__).app_context():
    data = trace()
    result = [acc(data, key) for key, _ in data.items() if not serializable(data[key])]
```

**Expected**

```python
result
>>> []
```

**Actual**

```python
result[0][0]
(
  'aide_logement_loyer_plafond<2017-12>',
  'prestations.aides_logement.loyers_plafond.par_zone<2017-12-01>',
  array([51.54]),
  numpy.ndarray),
  )
```
@bonjourmauko bonjourmauko requested a review from a team July 16, 2019 18:13
@bonjourmauko bonjourmauko added the kind:fix Bugs are defects and failure demand. label Jul 16, 2019
@bonjourmauko bonjourmauko force-pushed the fix/api-tracer-serialize branch from d71fffc to 23ac623 Compare July 16, 2019 18:30
@fpagnoux
Copy link
Member

fpagnoux commented Jul 16, 2019

Darn, this should have been caught by tests 🤦‍♂. Thanks for catching that.

Rather than reverting and re-reverting, I'd suggest:

  • Unpublish corrupt versions
  • Work on a fix
  • Publish fix

I think it'd we less work and cleaner. I can work on the fix right now.

@bonjourmauko
Copy link
Member Author

bonjourmauko commented Jul 16, 2019

Thanks @fpagnoux. I tried unpublishing with openfisca-bot, but the ”manage” button was greyed-out.

@fpagnoux fpagnoux mentioned this pull request Jul 16, 2019
@fpagnoux
Copy link
Member

Alternative fix without revert here: #900.
It solves the issue locally, but an extra check would be safer 🙂

@bonjourmauko bonjourmauko deleted the fix/api-tracer-serialize branch November 23, 2019 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:fix Bugs are defects and failure demand.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Serialization error in Web API with numpy arrays Erreur de serialisation de certaines paramètres (API)
3 participants