Simplify boilerplate

[fpagnoux]

• Move base.py content (file usually located in country packages) to core module formula_toolbox so that it can be reused by all countries
• Use AbstractScenario if no custom scenario is defined for a tax and benefit sytem

See impact on openfisca/openfisca-france#697

[fpagnoux]

If you have better ideas for the name of the module, feel free.

[MattiSG]

If formula_toolbox only exposes some very common functions as globals in order to avoid repetitive imports, then what about naming it shortcuts? Or imports?
If it constitutes all of what is used to create formulas, then what about naming it formula_api?

[MattiSG]

Why are there these awful # noqa on each line? 😷

[fpagnoux]

Otherwise the flake8 linter doesn't pass as we import stuff we do not use.

And no, there is no way to deactivate a specific error in a specific file. The only options I found are :

• Deactivating a specific error for the whole project
• Deactivating linting on a given line with noqa

@cbenz if you know a better way, I'll happily take it

[cbenz]

The better way would be to stick to the good practices, which are to import only what is needed by the code of the module.

As now, some Python modules are used as "import forwarders", letting other modules to import them in order to benefit from the imports they made... wow!

Adhering to these good practices will make this behavior forbidden.

But this is not in the scope of this PR.

[benjello]

To avoid warnings in some editors ...

[fpagnoux]

It constitutes all what is needed to write variables and formulas.

[cbenz]

What I explained in my comments has some impact on many modules of France.

But I think the path that was taken is not the best, to solve the problem of the boilerplate code.

I think the problem is on France: do we want all the model.* modules to import all the columns, periods, etc. or not ? My answer is yes, because this will lead to a clean surface API of OpenFisca, rather than hiding it under an import * statement.

[cbenz]

I'm not fond of the naming formula_toolbox (like other variants like _helpers or _functions).

I would prefer to name the module after what's inside.

Second, this module contains only imports, which is weird. I would prefer to import directly these modules from the impacted countries (like here)

But this is transitive and does not stop there! I mean openfisca_france/model/base.py should not exist neither lines like from openfisca_france.model.base import *.

We've got to do a choice: either we keep these "forwarding-only modules", either we forbid them and make real imports from modules.

[cbenz]

The better way would be to stick to the good practices, which are to import only what is needed by the code of the module.

As now, some Python modules are used as "import forwarders", letting other modules to import them in order to benefit from the imports they made... wow!

Adhering to these good practices will make this behavior forbidden.

But this is not in the scope of this PR.

[cbenz]

Why do that?

[fpagnoux]

Because right now the AbstractScenario is not really abstract, and can do the job without being overloaded.
Without it, the boilerplate country package needs to create its own Scenario, which only contains:

from openfisca_core.scenarios import AbstractScenario
class Scenario(AbstractScenario):
     pass

[fpagnoux]

I think the problem is on France: do we want all the model.* modules to import all the columns, periods, etc. or not ? My answer is yes

My answer would be no 😈. I think it's interesting to offer formula writers a bunch of tools to write formulas, without forcing them to think about where they come from.

I've worked with developers from local collectivities, and they are usually not python experts. It's more trouble for them to dig into the openfisca_core modules to find what they are looking for. I don't even really want them to know what core is precisely. But I can easily tell them : this is how you write formulas, it's documented here, and here are the objects/constants/functions you may want to use.

because this will lead to a clean surface API of OpenFisca, rather than hiding it under an import * statement.

I'd argue the opposite. This module would actually be our clean surface api to write formulas. The promise is that this module contains all what you need to do so. And the bad practise would be to dig deeply into openfisca_core to get objects you're not supposed to use.

[cbenz]

This module would actually be our clean surface api to write formulas

An import-forwarding module is not a clean surface API, it's hiding things under a carpet ;-)

And the bad practise would be to dig deeply into openfisca_core to get objects you're not supposed to use.

A good surface API would allow users to do from openfisca_core import (Variable, EnumCol, periods). Constants like ADD could be hosted under Variable, etc, etc. That would allow users to copy-paste a block of imports in their model modules which would be small and satisfy good practices, linting tools and wouldn't oblige to use # noqa.

Think of from numpy import (x, y, z), or import numpy as np, whereas numpy does not host all its functions in its __init__.py :-).

---

The fact is that, given the current state of the code, my comment is not really concrete.

In short, and given the constraints, at least I'd like to replace the indirection openfisca_france.model.x -> openfisca_france.model.base -> openfisca_core.formulas_toolbox by openfisca_france.model.x -> openfisca_core.imports_for_model

And rename formulas_toolbox to imports_for_model to make it explicit, and more exact (it's targeted to model modules which contain formulas and input variables :-))

I'm approving this PR, but I'd like one day to address those questions.
🎉

[fpagnoux]

I'll adapt the boiler plate country to 6.0.0 for now, so that we get more time to discuss the issue.

[fpagnoux]

As now, some Python modules are used as "import forwarders", letting other modules to import them in order to benefit from the imports they made... wow!

Think of from numpy import (x, y, z), or import numpy as np, whereas numpy does not host all its functions in its __init__.py :-).

Yep, and to do this, numpy actually does... import forwarding 😄 .

A good surface API would allow users to do from openfisca_core import (Variable, EnumCol, periods). Constants like ADD could be hosted under Variable, etc, etc. That would allow users to copy-paste a block of imports in their model modules which would be small and satisfy good practices, linting tools and wouldn't oblige to use # noqa.

"Copy paste a block" is an expression that raises huge alerts for me. Given the big number of imports we use to write formulas, that'd be a big block.

I understand that from x import * is a bad python practise, but model is not really a python file : it's an openfisca model file, basically a DSL. As @MattiSG mentionned, we could even have dependency injection. So to me it's fine to claim that in our context some python good practises are not really relevant.

And rename formulas_toolbox to imports_for_model to make it explicit, and more exact (it's targeted to model modules which contain formulas and input variables :-))

I would go for model_api (as what we offer is not just about formulas). It contains a promise which brings some value : this is all what you need to build your model files.

In short, and given the constraints, at least I'd like to replace the indirection openfisca_france.model.x -> openfisca_france.model.base -> openfisca_core.formulas_toolbox by openfisca_france.model.x -> openfisca_core.imports_for_model

The only difficulty is that you need two things to write formulas :

• The core objects/functions/classes offered by the model_api module discussed here
• The entities defined in each tax and benefit system.

The only role of openfisca_france.model.base is to combine these two. We could replace it by two lines of import in each model fine, but once more time I'd arbitrate towards simplicity for the formula writers, and keep the base module (and maybe rename it to france.model_api.

[fpagnoux]

To sum up:

• It seems that import forwarding is not unnatural in our case, as we just gather various objects we want to expose in a single module to be imported by users.

• The argument we have is more about the import * . However:

  • This PR doesn't add any import * to core
  • The import * already exist in france. So removing them could be another refactor if we decide to, but this PR is just keeping the situation as is.

To make the boilerplate country work on an official version of core, I'd like to merge this PR now, and let the door open to further improvements.