Parse parameters metadata and expose them in Python #681

fpagnoux · 2018-06-25T22:37:20Z

Depends on openfisca/country-template#49
Connected to #677

Expose Parameters metadata in the Python API
- Parameters unit and reference:
  - e.g. parameters.taxes.rate.unit, parameters.taxes.rate.reference
- Parameter value unit and reference:
  - e.g. parameters.taxes.rate.values_list[0].unit, parameters.taxes.rate.values_list[0].reference
- Parameter node description and reference:
  - e.g. parameters.taxes.reference, parameters.taxes.description
  - Note: Parameter descriptions (e.g. parameters.taxes.rate.description) were already exposed

benjello · 2018-06-26T14:07:23Z

It does the job. Thanks @fpagnoux !

MattiSG

I haven't tested so I can't approve, but the code reads good.

MattiSG · 2018-06-27T06:04:49Z

openfisca_core/parameters.py

@@ -778,11 +799,17 @@ def _validate_parameter(parameter, data, data_type = None, allowed_keys = None):
                    )


+def _item_to_list(item):


This is usually called wrap 🙂
Could it fit in a more generic helpers context?

MattiSG · 2018-06-27T06:05:23Z

setup.py

@@ -7,7 +7,7 @@

 setup(
    name = 'OpenFisca-Core',
-    version = '23.1.4',
+    version = '23.1.5',


I would favour a minor bump for such a new feature.

This is not just romance: I could write code that uses this new feature, and it is important for me to know that it needs v23.2, not a patch of v23.1.

MattiSG · 2018-06-27T06:47:39Z

tests/core/parameter_validation/filesystem_hierarchy/taxes/rate.yaml

@@ -0,0 +1,8 @@
+description: Tax rate
+unit: /1
+reference: http://legifrance.fr/taxes/rate


This URL doesn't exist. As it is just there for testing, https://law.gov.example/taxes/rate would be more explicit to me 🙂

benjello · 2018-06-29T12:34:05Z

@fpagnoux shouldn't you enforce the equality of unit and reference in Parameter.__eq__ ?

Anna-Livia · 2018-07-04T15:41:17Z

tests/core/parameter_validation/test_parameter_validation.py

+
+
+def test_parameters_metadata():
+    path = os.path.join(BASE_DIR, 'filesystem_hierarchy')


Why not use the country template for these tests ?
e.g.

import openfisca_country_template tbs = openfisca_country_template.CountryTaxBenefitSystem() basic_income = tbs.parameters.benefits.basic_income assert_equals(basic_income.description, 'Amount of the basic income')

Anna-Livia · 2018-07-04T15:42:11Z

tests/core/parameter_validation/test_parameter_validation.py

+    assert_equals(parameters.taxes.rate.values_list[0].reference, ['http://legifrance.fr/taxes/rate/2015-12'])
+    assert_equals(parameters.taxes.rate.values_list[0].unit, '/1')
+
+


I see a test for parameter, parameter_node, and parameter_at_instant and not for scale. Is that intentional ?

Anna-Livia · 2018-07-04T16:00:47Z

openfisca_core/parameters.py

-            data.pop('reference', None)
-            data.pop('description', None)
+            # We allow to set a reference and a description for a node.
+            self.reference = _item_to_list(data.pop('reference', None))


Why is it data.pop() here when it is data.get()in the others ?

Because in a parameter node, metadata and children are at the same level (see discussion about a children key here).

In the line that follows, we treat data as a dict of children parameters, so we need to "pop" the metadata out of the dict so that they are not interpreted as a child.

fpagnoux · 2018-07-05T19:45:16Z

@fpagnoux shouldn't you enforce the equality of unit and reference in Parameter.__eq__ ?

It seems that this custom equality is only used for tests, and that other metadata (like the description) were already ignored, so it doesn't seem super-necessary to me.

benjello · 2018-07-06T08:45:19Z

I am more enclined to use currency ou currnecy/EUR than EUR as a unit as it is more country package independent. See openfisca/openfisca-france#1022 for an explanation.

benjello · 2018-07-11T11:59:30Z

@fpagnoux : we should deal with the Scales as mentionend in openfisca/openfisca-france#1019.

Could you take care of that (consensus + implementation) ?
I would like to merge it ASAP to deal with downhill code.

Thanks !

fpagnoux · 2018-07-11T19:38:19Z

Sorry to come back on an already approved PR, but several discussions and new needs being expressed convinced me that our previous approach was a dead-end.

First, we can't reasonably list all metadata that users may one day want to add to parameters. For instance:

Marginal scales don't need a unique unit field, but two fields threshold_unit and rate_unit. See discussion[FR].
Users are interested in more advanced parameters.
The IPP would like to add the day of publishing of parameters in the "Journal officiel". This is valuable information, but only makes sense for France and should not be coded in Core.

Second, as we add more metadata, we are also increasing the possibility of collisions between them and sub-parameters, which could lead to headaches.

I'm therefore suggesting a different approach, by grouping all metadata in a metadata object, where legislation coders would be able to add custom metadata.

MattiSG · 2018-07-12T00:09:14Z

Assessment

I feel there are two distinct problems:

Normalising standard metadata. These should be thoroughly documented, and can open the way to Core handling such as type validation, presence check (reference…), or behaviour change (how to handle mismatching definition periods…).
Exposing arbitrary metadata without any filter. This could be offered as well, but opens room to divergence and typos (unti would be exposed as metadata.unti rather than raising an error).

Opinion

I am in favour of supporting both.

Arbitrary metadata exposure allows innovation to come from any country package, and can be normalised after the fact and moved to Core with top-level exposure.
Ensuring fully-specified and normalised metadata can be accessed allows cross-packages interoperability.

In the current case, that means to me proceeding with exposing unit at top-level with solid consensus on the format.

Immediate action suggestion

Since @benjello's need seems to be urgent, it might be beneficial to prioritise delivery of the arbitrary, grouped metadata pass-through. We could learn from actual usage the difficulties involved, and move slowly towards normalisation.

Long-term vision

I see how the Core metadata addition process could evolve to having convergent implementation of arbitrary metadata in two or three independent country packages.

For example, I see how reference is surprisingly not mature yet: while the need is obvious everywhere, the exact format of that property is not.
Aotearoa New Zealand has an interesting taxonomy for them, and France has mixed URLs and identifiers.
I feel the best would be to expose reference under metadata until a piece of work is seriously done on defining and unifying reference management, including providing tools to assess how properly referenced a country package is.

Anna-Livia · 2018-07-12T14:11:21Z

CHANGELOG.md

+Before:
+```YAML
+description: Age of retirement
+reference: https://wikipedia.org/wiki/retirement


Would it be interesting to give an example with several references ?

What would such an example illustrate?

Right now the metadata are just a pass-trough, and core has no opinion about the content of this reference, so I'm not sure about the value of making the changelog more complex by adding an other slightly different example.

I was asking because it was the first thing that came into my mind when I saw the singular reference and that there was only one reference :)
It might just be a worry I have :)

Yep, we should keep that in mind.

Anna-Livia · 2018-07-13T12:27:53Z

openfisca_core/parameters.py

        if data.get('values'):
-            _validate_parameter(self, data, allowed_keys = set(['values', 'description', 'unit', 'reference']))
+            # 'unit' and 'reference' are only listed here for backward compatibility
+            _validate_parameter(self, data, allowed_keys = set(['values', 'description', 'metadata', 'unit', 'reference']))


Why isn't descriptionconsidered a metadata in the new schema ?

Copied from above

The idea is to do a minimal change to the actual situation.
And description can be thought as standard/generic and informative enough
to be placed outside metadata which is more for custom/non-vital metadata.
But @fpagnoux may have more details

I actually hesitated while implementing.

The main argument that push to let description at the top level is that in Matti's comment, description seem to perfectly fit the definition of a "fully-specified and normalised metadata":

We have no interrogation about it, and we use it in many places, including on our parameters/ API route.

parameter.description has always been exposed

Now, if description is at top level, why not reference?

It's arguable, but if everyone is convinced that references are needed, it seems like we are not fully sure about what a reference should be exactly. And whenever we get a better idea about that, moving reference from metadata to top level is not even a breaking change for Core.

My main issue is constistency between where the description is and where the references are.
Would it be a big issue to break core and change parameter.description to parameter.metadata.description ?

I understand the argument, but if we want to have both "normalized and standard" and "free custom" metadata, it's hard to have both of them in the same place:
parameter.metadata['description'] would not look very normalized, would not protect us against typos in normalized fields, and would not make any distinction between standard and custom fileds .

Do you disagree with the general dichotomy of normalized vs custom?

I don't disagree with the paradigm, I think it is interesting to leave room for growth.
However, I think we need to create some kind of guidelines in the documentation for new comers and a timeline on how and when things should be normalized so that we can count on some formating standard to build cool tools that every country can use.

I'd rather have normalisation usage-based over time-based 🙂

Anna-Livia · 2018-07-13T12:30:08Z

openfisca_core/parameters.py


+        # If metadata have been provided


I am confused by this comment, as I read the following line as If values have been provided

Yep, this was very confusing. Adding more context :)

fpagnoux force-pushed the parse-metadata branch from cb8527f to 88f2b85 Compare June 25, 2018 22:40

fpagnoux requested review from Anna-Livia and sandcha June 25, 2018 22:40

benjello mentioned this pull request Jun 26, 2018

Renseigner une unité pour les seuils dans les barèmes (Scale) openfisca/openfisca-france#1019

Closed

fpagnoux force-pushed the parse-metadata branch from 6f6d1cc to 079feed Compare June 26, 2018 22:47

MattiSG mentioned this pull request Jun 27, 2018

Make parameters node accessible through the Web API #678

Closed

MattiSG reviewed Jun 27, 2018

View reviewed changes

benjello mentioned this pull request Jun 28, 2018

Complete units openfisca/openfisca-france#1022

Merged

7 tasks

benjello mentioned this pull request Jul 2, 2018

Add a unit to every legislative parameter openfisca/country-template#48

Closed

Anna-Livia requested changes Jul 4, 2018

View reviewed changes

fpagnoux force-pushed the parse-metadata branch 2 times, most recently from d2d445d to 12a2de5 Compare July 6, 2018 00:15

fpagnoux requested review from Anna-Livia and removed request for sandcha July 6, 2018 00:22

fpagnoux force-pushed the parse-metadata branch 2 times, most recently from 60b2d86 to 10b9bb2 Compare July 6, 2018 00:26

Anna-Livia approved these changes Jul 6, 2018

View reviewed changes

bonjourmauko added the flow:contrib:doing label Jul 6, 2018

bonjourmauko assigned Anna-Livia Jul 6, 2018

fpagnoux requested review from MattiSG and Anna-Livia July 11, 2018 19:38

fpagnoux self-assigned this Jul 11, 2018

MattiSG removed their request for review July 12, 2018 00:26

Anna-Livia requested changes Jul 13, 2018

View reviewed changes

fpagnoux requested a review from Anna-Livia July 13, 2018 15:17

Anna-Livia approved these changes Jul 16, 2018

View reviewed changes

fpagnoux force-pushed the parse-metadata branch from 47a0d6b to 8eb2b2c Compare July 16, 2018 17:26

fpagnoux added 20 commits July 16, 2018 13:26

Parse unit and reference for parameters

7d032eb

Parse unit and reference for ParameterAtInstant

c5f5fd4

Sort children values in Parameter.__repr__

4a70d6a

Remove warning in tests

26750dd

Inherit unit from Parameter to ParameterAtInstant

f0e4edd

Parse node metadata

31781dc

Make reference a list (like with variables)

4705ffa

Add metadata for scales

2cba8c4

Bump version number

93266fc

Check metadata in index.yaml

0ac2b39

Rename item_to_list to wrap

5d9a3dd

Use examples from country template

738936c

Introduce parameter.metadata

76fe026

Update Changelog

f797d26

Do not wrap references

6ddd606

Don't use an abstract class just to share 2 func

c9f731f

Use - as separator to avoid confusion with objects

032675b

Explicit simplified param declaration processing

1e903e6

Fix indentation of FB usage in circle conf

fbd8fb6

Upgrade country template

f292bf6

fpagnoux force-pushed the parse-metadata branch from c6409e6 to f292bf6 Compare July 16, 2018 18:55

fpagnoux merged commit 363de93 into master Jul 16, 2018

fpagnoux deleted the parse-metadata branch July 16, 2018 19:20

fpagnoux removed the flow:contrib:doing label Jul 16, 2018

bonjourmauko added the kind:feat A feature request, a feature deprecation label Jul 21, 2018

Morendil mentioned this pull request Mar 22, 2019

Make parameters metadata accessible through Python and the Web API #677

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse parameters metadata and expose them in Python #681

Parse parameters metadata and expose them in Python #681

fpagnoux commented Jun 25, 2018 •

edited

Loading

benjello commented Jun 26, 2018

MattiSG left a comment

MattiSG Jun 27, 2018

MattiSG Jun 27, 2018

MattiSG Jun 27, 2018

benjello commented Jun 29, 2018

Anna-Livia Jul 4, 2018

fpagnoux Jul 6, 2018

Anna-Livia Jul 4, 2018

Anna-Livia Jul 4, 2018

fpagnoux Jul 6, 2018 •

edited

Loading

fpagnoux commented Jul 5, 2018 •

edited

Loading

benjello commented Jul 6, 2018

benjello commented Jul 11, 2018

fpagnoux commented Jul 11, 2018

MattiSG commented Jul 12, 2018

Anna-Livia Jul 12, 2018

fpagnoux Jul 13, 2018

Anna-Livia Jul 13, 2018

fpagnoux Jul 13, 2018

Anna-Livia Jul 13, 2018

benjello Jul 13, 2018

fpagnoux Jul 13, 2018

Anna-Livia Jul 13, 2018 •

edited by fpagnoux

Loading

fpagnoux Jul 13, 2018 •

edited

Loading

Anna-Livia Jul 16, 2018

MattiSG Jul 18, 2018

Anna-Livia Jul 13, 2018

fpagnoux Jul 13, 2018

		@@ -778,11 +799,17 @@ def _validate_parameter(parameter, data, data_type = None, allowed_keys = None):
		)


		def _item_to_list(item):



		def test_parameters_metadata():
		path = os.path.join(BASE_DIR, 'filesystem_hierarchy')

		assert_equals(parameters.taxes.rate.values_list[0].reference, ['http://legifrance.fr/taxes/rate/2015-12'])
		assert_equals(parameters.taxes.rate.values_list[0].unit, '/1')

Parse parameters metadata and expose them in Python #681

Parse parameters metadata and expose them in Python #681

Conversation

fpagnoux commented Jun 25, 2018 • edited Loading

benjello commented Jun 26, 2018

MattiSG 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

benjello commented Jun 29, 2018

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 Jul 6, 2018 • edited Loading

Choose a reason for hiding this comment

fpagnoux commented Jul 5, 2018 • edited Loading

benjello commented Jul 6, 2018

benjello commented Jul 11, 2018

fpagnoux commented Jul 11, 2018

MattiSG commented Jul 12, 2018

Assessment

Opinion

Immediate action suggestion

Long-term vision

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

Anna-Livia Jul 13, 2018 • edited by fpagnoux Loading

Choose a reason for hiding this comment

fpagnoux Jul 13, 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

fpagnoux commented Jun 25, 2018 •

edited

Loading

fpagnoux Jul 6, 2018 •

edited

Loading

fpagnoux commented Jul 5, 2018 •

edited

Loading

Anna-Livia Jul 13, 2018 •

edited by fpagnoux

Loading

fpagnoux Jul 13, 2018 •

edited

Loading