Remove metaclasses #388

fpagnoux · 2016-06-09T13:00:11Z

The initial goal of this PR was to get rid of the metaclass system. As is it pretty deep in the core of openfisca, a lot of other changes comes with it. Feedback and review from other contributors would be particularly appreciated here

Metaclass system

In the former system, openfisca variables were loaded into the tax benefit system statically through metaclasses. Concretely, when python was importing the different modules files, the variables would be automatically loaded in the entities classes. Then only the tax benefit system would be created, and would be able to access these variables through the entity classes.
This is pretty lopsided for many reasons. The lifecyles of the different objects is pretty hard to handle, as many things happen at the loading of the python modules. This make it very hard for reforms to change the tax benefit system (hence the rough API).
This mechanism is removed with this PR. See next comment about the new TaxBenefitSystem API to check the new mechanism.

To do :

Update documentation
Remove loading extensions through git
Make update_variable more robust (see note)

So that it works when column is in tbs.columb_by_name. Deprecate simulations.self.entity_by_column_name Introduce simulations.getVariableEntity

In formulas.add_column_to_tax_benefit_system For temp retrocompatibility reason This function will disappear anyway

fpagnoux · 2016-06-09T13:00:45Z

Creating a tax benefit system.

A tax benefit system is a set of :

Variables (some of them calculable)
Entities for which these variables are defined
Legislation parameters

Example

# This hasn't change: I need to create my entities and scenario.

class Scenario(AbstractScenario):
    #...

class Familles(AbstractEntity):
    #...

class Individus(AbstractEntity):
    #...


# Variable classes can still inherit Variable, DatedVariable, EntityToPersonColumn or PersonToEntityColumn.
class age_en_mois(Variable):
    #...

class birth(Variable):
    #...

class salaire_brut(Variable):
    #...

# I want to define a particular tax benefit system.
class TestTaxBenefitSystem(TaxBenefitSystem):

    # The construction of the TBS is done in its constructor.
    def __init__(self):

        # I first need to call the parent constructor for a basic initialization, providing the entities in a simple list.
        entities = [Familles, Individus]
        TaxBenefitSystem.__init__(self, entities)

        self.Scenario = Scenario

        # I add a variable to the TBS
        self.add_variable(age_en_mois)

        # I add several variables at the same time.
        self.add_variables(birth, salaire_brut)

        # I add all the ones declared in a python file
        self.add_variables_from_file("/path/to/openfisca_variables.py")

        # I recursively browse a directory to add all the variables declared in it.
        self.add_variables_from_directory("other/path/to/openfisca/variables/")

        # I declare the main parameter file
        path_to_root_params = 'path/to/root_params.xml'
        self.add_legislation_params(path_to_root_params)

        # I add another parameter file
        path_to_crds_params = 'path/to/crds_params.xml'
        # It will be added to the tree into the node `csg.activite`
        self.add_legislation_params(path_to_crds_params, 'csg.activite')

# I can then use it
tbs = TestTaxBenefitSystem()

# Will return the appropriate column
tbs.get_column('birth')

# Will compute and return the legislation JSON
tbs.get_legislation()

# Will return the more easily browsable legislation for a given time.
legislation = tbs.get_compact_legislation(Instant((2013, 1, 1)))
montant_al = legislation.al.montant

benjello · 2016-06-09T13:19:28Z

Very nice work ! Congratulations !
Everything is simpler and clearer.
Did you work on the reform (parametric, structural) par too ?

fpagnoux · 2016-06-09T13:28:36Z

Creating a reform

A reform is a usually small set of modifications applied to a tax benefit systems (i.e. to its parameters or variables), in order to study the impact of legislation modification.

The way to build them changes significantly with this PR.

Example:

# I declare a new variable

class charge_loyer(Variable):
    column = columns.FloatCol
    entity_class = entities.FoyersFiscaux
    label = u"Charge déductible pour paiement d'un loyer"

    def function(self, simulation, period):
        return period, (...)

# I declare another one aiming at replacing an existing variable

class charges_deduc(Variable):
    label = u"Charge déductibles intégrant la charge pour loyer (Trannoy-Wasmer)"
    reference = "charges_deduc"

    def function(self, simulation, period):
        return period, (...)

# I declare a legislation modifier. The API hasn't changed and is still rough to handle.
def modify_legislation_json(reformed_legislation):
    reform_legislation_subtree = {
        "@type": "Node",
        "description": "Charge de loyer",
        "children": {...},
        }
    reformed_legislation['children']['charge_loyer'] = reform_legislation_subtree
    return reformed_legislation


# I can finally declare my reform
class trannoy_wasmer(Reform):
    name = u'Loyer comme charge déductible (Trannoy-Wasmer)'

    # I need to implement this method.
    # Its role is to transform the reference tax benefit system into the reformed one.
    # When this method is called, self is a copy of the reference tax benefit system.
    def apply(self):

        # I add a new variable
        self.add_variable(charge_loyer)

        # I replace an existing one
        self.update_variable(charges_deduc)

        # I modify the legislation
        self.modify_legislation_json(modifier_function = modify_legislation_json)

        # I can also neutralize a column
        self.neutralize_column('rsa')

# I can then use my reform

tbs = SomeTaxBenefitSystem()

# I can my slightly different tax benefit system
reformed_tbs = trannoy_wasmer(tbs)

benjello · 2016-06-09T13:33:05Z

Looks very good, indeed.
Did you take car of the reference attribute etc so the simulation.calculate can handle both the reform and the original tbs ?
Sorry for dropping question without testing but i do not have much time right now but i will definitively
try your PR my tbs and reform.

fpagnoux · 2016-06-09T13:33:15Z

Some code has been removed, as was either already said to be deprecated, or didn't seem to be used:

XmlBasedTaxBenefitSystem
update_legislation and updated_legislation_items in reforms.

fpagnoux · 2016-06-09T13:35:41Z

@benjello yep as you can see here you can still use reference = True to simulate without taking the reform into account.

fpagnoux · 2016-06-09T13:39:54Z

No worries this has big impacts on the API so all questions and feedback are really welcome so that we end up with something satisfying for everyone ;)

benjello · 2016-06-09T13:45:03Z

If update_legislation and updated_legislation_items were not replaced I may still use them ...
I will give you feedback ASAP.

fpagnoux · 2016-06-09T14:18:13Z

Creating an extension:

An extension is a set of variables and parameters that can be added to a tax benefit system, but which purpose is too specific to be added to the general shared code base of this tax benefit system (e.g. Paris social prestations in openfisca-france)

An extension is loaded this way:

tbs = SomeTaxBenefitSystem()
tbs.load_extension('path/to/extensions/folder')

The extension folder must have a specific architecture:

    extension_folder/parameters.xml # Optional parameters file. Must be called `parameters.xml
    extension_folder/{some_formula}.py # File containing formulas
    extension_folder/{other_formula}.py
    extension_folder/{some_formula}.yaml # Optional test files
    extension_folder/{other_formula}.yaml

laem · 2016-06-09T16:13:33Z

🎉

+* XmlBasedTaxBenefitSystem is deprecated, and MultipleXmlBasedTaxBenefitSystem renamed to TaxBenefitSystem

👍
I'll have to adapt #387 following the same principles.

I declare a legislation modifier. The API hasn't changed and is still rough to handle.
def modify_legislation_json(reformed_legislation):

This should be simplified by YAML params.

cbenz · 2016-06-10T09:52:44Z

Wow it's a good design, I especially like the add_variables_from_file / add_variables_from_directory methods.

There are still Variable classes with a to_column method.

cbenz · 2016-06-10T09:54:42Z

openfisca_core/scenarios.py

@@ -185,7 +185,7 @@ def fill_simulation(self, simulation, use_set_input_hooks = True, variables_name
                    # All parallel axes have the same count and entity.
                    first_axis = parallel_axes[0]
                    axis_count = first_axis['count']
-                    axis_entity = simulation.entity_by_column_name[first_axis['name']]
+                    axis_entity = simulation.getVariableEntity(first_axis['name'])


In Python the convention would be camel case: get_variable_entity :)

cbenz · 2016-06-10T09:56:34Z

Also, very good to get rid of Simulation.entity_by_column_name and Entity.column_by_name

cbenz · 2016-06-10T10:06:57Z

openfisca_core/taxbenefitsystems.py

+            import tempfile
+            import subprocess
+            temp_dir = tempfile.mkdtemp()
+            clone_command = ' '.join(['git clone', path, temp_dir, '&> /dev/null'])


Waw that's perhaps not the right place :)

I guess the usage is:

tbs.load_extension('https://github.com/xxx/yyy.git')

I'd like to decouple the git clone from the loading of the extension.

We can do:

$ pip install git+https://github.com/xxx/yyy.git

then

tbs.load_extension_from_package('yyy')

where yyy is a Python package name.

Yay I admit this may be a little weird. I thought I would need it, but actually I'm not using it, so I'll just remove the feature. Adding extensions from a package seems interesting too, I'll keep that in mind.

fpagnoux · 2016-06-10T16:06:48Z

@benjello I don't have a strong opinion about update_legislation and updated_legislation_items. They were marked as deprecated so I thought they were not used anymore. If you want, let me know and I'll put them back, as anyway @laem will probably offer new helpers when he is done working on the parameters.

fpagnoux · 2016-06-10T16:09:01Z

openfisca_core/scenarios.py

@@ -185,7 +185,7 @@ def fill_simulation(self, simulation, use_set_input_hooks = True, variables_name
                    # All parallel axes have the same count and entity.
                    first_axis = parallel_axes[0]
                    axis_count = first_axis['count']
-                    axis_entity = simulation.getVariableEntity(first_axis['name'])


@cbenz thanks for the quick fix.

fpagnoux · 2016-06-13T11:55:07Z

This seems GTM to me. @benjello what about update_legislation and updated_legislation_items ?

Deprecate AbstractReform

@cbenz

From a @cbenz commit erased my mistake

laem · 2016-06-21T08:15:24Z

setup.cfg

@@ -29,6 +29,6 @@ previous = true
 hang-closing = true
 ; E128 continuation line under-indented for visual indent
 ; E251 unexpected spaces around keyword / parameter equals
-ignore = E128,E251
+ignore = E128,E251,F405


Is there a description of F405 to add before ?

F405 name may be undefined, or defined from star imports: module

fpagnoux added 6 commits June 6, 2016 16:14

Refactor get_or_new_holder

db71b54

So that it works when column is in tbs.columb_by_name. Deprecate simulations.self.entity_by_column_name Introduce simulations.getVariableEntity

First non-metaclass variable

9871dac

Put all FormulaColumnMetaclass logic in variables

977fcc0

Uncomment reference test

749a5f9

Loosen behaviour if columns already exists

906f969

In formulas.add_column_to_tax_benefit_system For temp retrocompatibility reason This function will disappear anyway

Make get_column retrocompatible

6378991

fpagnoux force-pushed the remove-metaclasses branch from 7cb2a4e to b829b47 Compare June 9, 2016 13:11

fpagnoux mentioned this pull request Jun 9, 2016

Adapt france to core 2.0 openfisca/openfisca-france#484

Merged

2 tasks

cbenz reviewed Jun 10, 2016
View reviewed changes

fpagnoux reviewed Jun 10, 2016
View reviewed changes

fpagnoux force-pushed the remove-metaclasses branch 3 times, most recently from 278595f to 9c03bf1 Compare June 13, 2016 09:04

fpagnoux added 10 commits June 15, 2016 14:46

Add tbs.add_variables_from_directory

e55fcc7

Improve add_variables_from_module

32a60ce

Deprecate XmlBasedTaxBenefitSystem

c76fa07

Load parameters dynamically

93b68bb

Simplify entity declaration

2e71000

Update tests to new api

858bfbd

Enable extension loading

e41f213

Test using a dummy extension

706e5f9

Introduce new reform API

ff58441

Change reform api

9e96f94

fpagnoux force-pushed the remove-metaclasses branch from eec16f4 to e0b68ad Compare June 15, 2016 12:47

fpagnoux added 7 commits June 15, 2016 14:48

Deprecate old reform API

f2c627a

Deprecate AbstractReform

Rename NewReform to Reform

33a302e

Rename replace_variable -> update_variable

2fe6d89

Fix compose reform test

8386c38

Avoid side effects in tests

eb135ba

Add test for modifying legislation

a1a40d5

Change style to initialize a TBS

92d95c2

fpagnoux force-pushed the remove-metaclasses branch from e0b68ad to e6b58f5 Compare June 15, 2016 12:48

fpagnoux added 2 commits June 15, 2016 15:17

Remove redundant reference attribute

51e532a

Minor fixes

0c48545

From a @cbenz commit erased my mistake

fpagnoux force-pushed the remove-metaclasses branch from e6b58f5 to 2a250c7 Compare June 15, 2016 13:17

Bump version number and update CHANGELOG.md

0a07fcd

fpagnoux force-pushed the remove-metaclasses branch from 2a250c7 to 0a07fcd Compare June 20, 2016 16:03

Ignore F405 style error

aa3ddc7

fpagnoux merged commit 39eae3f into master Jun 20, 2016

fpagnoux deleted the remove-metaclasses branch June 20, 2016 16:50

laem reviewed Jun 21, 2016
View reviewed changes

This was referenced Jul 5, 2016

Adapt API to Openfisca 2.0 openfisca/openfisca-web-api#57

Merged

Adapt parsers to Openfisca 2.0 openfisca/openfisca-parsers#11

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove metaclasses #388

Remove metaclasses #388

fpagnoux commented Jun 9, 2016 •

edited

Loading

fpagnoux commented Jun 9, 2016 •

edited

Loading

benjello commented Jun 9, 2016

fpagnoux commented Jun 9, 2016 •

edited

Loading

benjello commented Jun 9, 2016

fpagnoux commented Jun 9, 2016

fpagnoux commented Jun 9, 2016 •

edited

Loading

fpagnoux commented Jun 9, 2016

benjello commented Jun 9, 2016

fpagnoux commented Jun 9, 2016

laem commented Jun 9, 2016

cbenz commented Jun 10, 2016 •

edited

Loading

cbenz Jun 10, 2016

cbenz commented Jun 10, 2016

cbenz Jun 10, 2016

fpagnoux Jun 10, 2016

fpagnoux commented Jun 10, 2016

fpagnoux Jun 10, 2016

fpagnoux commented Jun 13, 2016 •

edited

Loading

laem Jun 21, 2016

benjello Jun 21, 2016

Remove metaclasses #388

Remove metaclasses #388

Conversation

fpagnoux commented Jun 9, 2016 • edited Loading

Metaclass system

fpagnoux commented Jun 9, 2016 • edited Loading

Creating a tax benefit system.

benjello commented Jun 9, 2016

fpagnoux commented Jun 9, 2016 • edited Loading

Creating a reform

benjello commented Jun 9, 2016

fpagnoux commented Jun 9, 2016

fpagnoux commented Jun 9, 2016 • edited Loading

fpagnoux commented Jun 9, 2016

benjello commented Jun 9, 2016

fpagnoux commented Jun 9, 2016

Creating an extension:

laem commented Jun 9, 2016

cbenz commented Jun 10, 2016 • edited Loading

cbenz Jun 10, 2016

Choose a reason for hiding this comment

cbenz commented Jun 10, 2016

cbenz Jun 10, 2016

Choose a reason for hiding this comment

fpagnoux Jun 10, 2016

Choose a reason for hiding this comment

fpagnoux commented Jun 10, 2016

fpagnoux Jun 10, 2016

Choose a reason for hiding this comment

fpagnoux commented Jun 13, 2016 • edited Loading

laem Jun 21, 2016

Choose a reason for hiding this comment

benjello Jun 21, 2016

Choose a reason for hiding this comment

fpagnoux commented Jun 9, 2016 •

edited

Loading

fpagnoux commented Jun 9, 2016 •

edited

Loading

fpagnoux commented Jun 9, 2016 •

edited

Loading

fpagnoux commented Jun 9, 2016 •

edited

Loading

cbenz commented Jun 10, 2016 •

edited

Loading

fpagnoux commented Jun 13, 2016 •

edited

Loading