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

Semantic versioning usability in OpenFisca extension #10

Closed
guillett opened this issue Apr 12, 2018 · 10 comments
Closed

Semantic versioning usability in OpenFisca extension #10

guillett opened this issue Apr 12, 2018 · 10 comments

Comments

@guillett
Copy link
Member

guillett commented Apr 12, 2018

How to leverage semantic versioning without a proper dependency solver ?

As an extension developer, I struggle to manage version constraints.

  • Should an extension depend on its country package and core ?
    • How much to we want extension developer to know the limits between core and country ?
  • What is the public API of a country package ?

To me it looks like, currently, an extension developer needs to specify the exact country package version (at least on OpenFisca-France) because event a patch update tolerance can lead to incompatibility issues.

Here is the experiment I have done (I uses French packages to illustrate my difficulties).

OpenFisca-France Changelog

18.4.2 - #770

  • Amélioration technique
  • Détails :
    • Mets à jour OpenFisca-Core
      • Modifie la manière de définir les dates de début des formules, et les dates de fin des variables.
      • Modifie les conventions de nommages des formules des variables.
      • Pour plus d'information, voir le Changelog d'OpenFisca-Core correspondant.

I created a package to test a way to manage version constraints.
I was hoping that the following settings would be correct and usable.

    install_requires=[
        'OpenFisca-Core >= 13, < 14',
        'OpenFisca-France >= 18, < 19',
    ],

However, it is not the case as shown by the runs on CircleCI.

Extract from pip list

OpenFisca-Core (13.0.1)
OpenFisca-France (18.11.1)
Openfisca-SEMVER (0.1.0)

The error, running ! openfisca-run-test test.yaml

Traceback (most recent call last):
  File "/home/ubuntu/virtualenvs/venv-2.7.12/bin/openfisca-run-test", line 11, in <module>
    sys.exit(main())
  File "/home/ubuntu/virtualenvs/venv-2.7.12/lib/python2.7/site-packages/openfisca_core/scripts/run_test.py", line 27, in main
    tax_benefit_system = build_tax_benefit_sytem(args.country_package, args.extensions, args.reforms)
  File "/home/ubuntu/virtualenvs/venv-2.7.12/lib/python2.7/site-packages/openfisca_core/scripts/__init__.py", line 27, in build_tax_benefit_sytem
    country_package_name = detect_country_package()
  File "/home/ubuntu/virtualenvs/venv-2.7.12/lib/python2.7/site-packages/openfisca_core/scripts/__init__.py", line 54, in detect_country_package
    module = importlib.import_module(module_name)
  File "/opt/circleci/python/2.7.12/lib/python2.7/importlib/__init__.py", line 37, in import_module
    __import__(name)
  File "/home/ubuntu/virtualenvs/venv-2.7.12/lib/python2.7/site-packages/openfisca_france/__init__.py", line 3, in <module>
    from france_taxbenefitsystem import FranceTaxBenefitSystem
  File "/home/ubuntu/virtualenvs/venv-2.7.12/lib/python2.7/site-packages/openfisca_france/france_taxbenefitsystem.py", line 8, in <module>
    from .entities import entities
  File "/home/ubuntu/virtualenvs/venv-2.7.12/lib/python2.7/site-packages/openfisca_france/entities.py", line 51, in <module>
    'label': u'Personnes à charge'
TypeError: build_entity() got an unexpected keyword argument 'doc'

Given the recent discussion on Slack about this (an important topic), I thought switching to GitHub is appropriate.

WDYT ? @MattiSG , @fpagnoux, @Morendil

@MattiSG
Copy link
Member

MattiSG commented Apr 12, 2018

What is the public API of a country package?

To me, it is the set of all its exposed parameters and variables, and all of its transitively exposed API surface (i.e. all helpers it includes and exposes, including Core-forwarded ones).

If that theoretical definition is agreed upon, then the referenced 18.4.2 example should have been a breaking change.

Should an extension depend on its country package and core?

To prevent conflicts, I would expect the Core dependency to be a transitive dependency from the Country package.

The main interest I see in making the Core dependency explicit would be to know immediately if a major Country upgrade implies a Core upgrade, without reading the changelog.

@guillett
Copy link
Member Author

Thanks @MattiSG for your answer.

What about OpenFisca-France 20.7.0

## 20.7.0 [#895](https://github.com/openfisca/openfisca-france/pull/895)

* Amélioration technique
* Détails :
  - Adapte France à OpenFisca Core v22

@fpagnoux did you change the syntax in OpenFisca-France because you had to (following the upgrade to Core v22) or because it was nicer?

Great thanks !

@fpagnoux
Copy link
Member

@fpagnoux did you change the syntax in OpenFisca-France because you had to (following the upgrade to Core v22) or because it was nicer?

Because I had, the previous trick was not compatible with the new version of Core.

To me, it is the set of all its exposed parameters and variables, and all of its transitively exposed API surface (i.e. all helpers it includes and exposes, including Core-forwarded ones).

So far, the second aspect has not entirely been taken into account, as we mainly focused on web API re-users.

However, if we were to take the definition literally, this probably means than every core breaking change would also be a breaking change for France:

  • Any change in the model syntax would also apply to extensions
  • If a method is public in Core, you can usually access it somehow from the TaxBenefitSystem object.

While frequent major versions publishing is not an issue per say (we don't do romantic versioning), two questions will arise:

  • How do we clearly distinguish, for our typical users that just do calculation (usually through the web API, self-hosted or public), changes that can impact them ? Today they just have to look for breaking change (BC) in the changelog. What will happen when a lot more BC that don't impact them appear in the Changelog ? (FI from francev18 to francev21, the core dependency moved from corev10 to corev22).
  • The versioning strategy of the public france web API is based on the major version number of france. This strategy will have to be rethought if BC become more frequent and irrelevant for the web-api.

@fpagnoux
Copy link
Member

fpagnoux commented Apr 12, 2018

I created a package to test a way to manage version constraints.
I was hoping that the following settings would be correct and usable.

    install_requires=[
        'OpenFisca-Core >= 13, < 14',
        'OpenFisca-France >= 18, < 19',
    ],

However, it is not the case as shown by the runs on CircleCI.

Extract from pip list

OpenFisca-Core (13.0.1)
OpenFisca-France (18.11.1)
Openfisca-SEMVER (0.1.0)

Just to clarify something about this example:

  • The constraint set has solutions : for instance france v18.4.0,
  • Pip is not able to find them, and ends up installing 2 incompatible librairies (france v18.11.1 needs core v20).

So (as of today) providing both dependencies is not enough for guaranteeing that the installation works.

@guillett
Copy link
Member Author

The solution would be :

OpenFisca-Core (13.0.1)
OpenFisca-France (18.4.1)

@MattiSG
Copy link
Member

MattiSG commented Apr 24, 2018

this probably means than every core breaking change would also be a breaking change for France

Fair point. That doesn't make a lot of sense either: the Country package might very well absorb the breaking change without any change to reusers, and the fact that extensions are in a weird situation where they depend both on Core functions and Country formulas is quite specific.
Considering when 18.4.2 was released, I guess the understanding I had at the time was “extensions should specify their own country package and Core dependencies, and the dependency solver should help them figure out if they rely on something incompatible”. That makes more sense to me than what I had described earlier.


It seems to me the question is now: can we consider a Country exposes an API surface of its own that can be considered different than the Core? This one is really tricky.

I would think that the only API surface exposed by the country package should be its formulas and parameters. Is this currently the case? What else is exposed, including transitively?

If the above was ensured, that would imply all helpers should be imported by extensions straight from the Core, not from the Country package.

This way:

  • A breaking change in the Core that impacts the signature of formulas or parameters would be a breaking change for the Country package.
  • A breaking change in the Core that impacts the way these formulas or parameters are declared (but not consumed) would not be a breaking change for the Country package, but would be detected by extensions as they would have an explicit dependency on the Core major version.

What I understand from the original post and #10 (comment), though, is that Pip shits its pants and “ends up installing 2 incompatible librairies”. If really the issue is only with the dependency solving in Pip, as @guillett pointed out (and that the current documentation + practice in OpenFisca is consistent and fits the criteria I described above), then the best is probably to just add a workaround (pointer: zazo) for the time until Pip fixes this issue, as seems to be underway, hopefully landing in a couple months.

@fpagnoux
Copy link
Member

I would think that the only API surface exposed by the country package should be its formulas and parameters. Is this currently the case? What else is exposed, including transitively?

If the above was ensured, that would imply all helpers should be imported by extensions straight from the Core, not from the Country package.

In the extension template, all helpers are directly imported from core.

For a web API user, the variables and parameters signatures, and the web API itself, constitute the surface API. A few example of breaking changes:

  • Coming from france itself:
    • A variable is renamed
    • A parameter is renamed
    • Some variable that used to accept yearly input values now can only been initiated monthly
  • Transitive breaking changes:
    • The way to set the value of a certain type of variable has changed (e.g. enums recently)
    • The structure of the object returned by the route /calculate has changed

For someone who uses OpenFisca in Python, much more is transitively exposed. For instance, let's considering this snippet:

from openfisca_france import CountryTaxBenefitSystem
tax_benefit_system = CountryTaxBenefitSystem()
print(tax_benefit_system.get_variable('salaire_net').value_type)

If the value_type attribute of a variable is renamed, this codes breaks. There are many similar examples with various Core modules. So, indirectly, a lot of the Core api is exposed, even importing only objects from openfisca_france!

So far, the choice was to optimize the France version number for the first kind of users. The second kind, like extensions developpers, usually control both their core and france dependencies.

@MattiSG
Copy link
Member

MattiSG commented May 15, 2018

@guillett what would solve the issue for you? Can we just document this understanding of what is the API surface in each relevant module? Or do you want support to try out zazo? 🙂

@fpagnoux
Copy link
Member

Pipenv could be interesting to dig to. We don't really care about the whole lockfile thing, but they claim to have a real dependency resolver, and they are packed by the Python Software Foundation.

@Morendil
Copy link
Contributor

Morendil commented Apr 9, 2019

Closing as stale. I think we're overdue a discussion about how we define the "public API" of the various parts of OpenFisca for the purposes of Semver, and this issue is not the best venue for this discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants