Adapte à Python 3 #980

fpagnoux · 2018-05-12T23:48:49Z

fpagnoux · 2018-05-12T23:49:31Z

circle.yml

@@ -9,7 +9,7 @@ dependencies:
  override:
    - pip install --upgrade pip twine wheel  # pip >= 8.0 needed to be compatible with "manylinux" wheels, used by numpy >= 1.11
    # Uncomment and adapt the next line to use a particular feature branch of OpenFisca-Core to run Circle CI tests
-    # - pip install --editable git+https://github.com/openfisca/openfisca-core.git@BRANCH_NAME#egg=OpenFisca-Core
+    - pip install --editable git+https://github.com/openfisca/openfisca-core.git@python3#egg=OpenFisca-Core


Needs to be reverted before merging

Anna-Livia

Est-ce que tous les tests passent en python3 avec ces modifications ? Certains des tests ?
Je pense qu'il serait interessant d'ajouter les tests en Python3 au circle pour eviter les regressions.

Anna-Livia · 2018-05-14T12:49:38Z

CHANGELOG.md

@@ -1,5 +1,11 @@
 # Changelog

+## 21.1.0 [#980](https://github.com/openfisca/openfisca-france/pull/980)


Si c'est une amélioration technique, pourquoi ne pas avoir 21.9.10 ?

Avant, je ne pouvais pas utiliser OpenFisca France en Python 3.
Maintenant, je peux.
C'est une nouvelle feature :).

Il n'y a pas a priori de lien direct entre la nature du changement et le type de version bump. Certaines évolutions de la législation sont juste des bug fix (telle formule donnait un mauvais résultat), certains changement techniques, comme celui-ci, apportent de nouvelles features.

Anna-Livia · 2018-05-14T13:05:45Z

...e/model/prelevements_obligatoires/prelevements_sociaux/cotisations_sociales/travail_prive.py

@@ -288,6 +288,7 @@ def formula(individu, period, parameters):
        agirc_salarie = individu('agirc_salarie', period)
        assiette_cotisations_sociales = individu('assiette_cotisations_sociales', period)
        categorie_salarie = individu('categorie_salarie', period)
+        plafond_securite_sociale = individu('plafond_securite_sociale', period)


🙌 Great catch ! 🙌
Comment est-ce que les tests arrivaient à passer avant ?
Comment as-tu identifié l'erreur ?

Avant, plafond_securite_sociale n'étant pas défini dans la formule, c'est la classe plafond_securite_sociale , définie dans le même fichier, qui était utilisée.

Or Python 2 ne lançait pas d'erreur quand on lui demandait de comparer une class à un float. Il devait sans doute renvoyer un résultat non pertinent, mais faute de test unitaire, ça n'a pas dû être détecté.

Python 3 n'accepte pas cette comparaison (et tant mieux!), donc je suis tombé sur une erreur bizarre en essayant de faire passer les tests.

Anna-Livia · 2018-05-14T13:27:14Z

openfisca_france/__init__.py

@@ -1,5 +1,5 @@
 # -*- coding: utf-8 -*-

-from france_taxbenefitsystem import FranceTaxBenefitSystem
+from .france_taxbenefitsystem import FranceTaxBenefitSystem


On avait statué sur les absolute import dans core il me semble. Pourquoi ne pas faire la même chose ici ?

L'enthousiasme de faire passer les tests en utilisant le moins de touches de clavier possible ?
Good catch 👍

fpagnoux · 2018-05-14T14:09:37Z

Est-ce que tous les tests passent en python3 avec ces modifications ? Certains des tests ?
Je pense qu'il serait interessant d'ajouter les tests en Python3 au circle pour éviter les regressions.

Tous les tests nose, pas encore vérifié pour les tests YAML.
Oui, il faudrait ajouter à Circle. Est-ce que quelqu'un sait comment faire et veut s'y coller 🙂 ?

Anna-Livia · 2018-05-14T15:23:34Z

Je dirait qu'il faudrait passer à circle2 en copiant grossièrement ce qui a été fait avec openfisca-core. Tu te sens de te lancer ?

Anna-Livia · 2018-05-14T15:25:02Z

openfisca_france/scenarios.py


-from model.base import *
+from .model.base import *


Absolute imports :)

Anna-Livia · 2018-05-14T15:25:29Z

tests/test_basics.py

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

 import datetime

-from cache import tax_benefit_system
+from .cache import tax_benefit_system


Absolute imports :)

The test package is not under openfisca_france, so I don't see how I can use an absolute import.

tests.cache ?

That would actually look a lot like a now forbidden implicit relative import!

The idea behind forbidding implicit relatives imports was that:

Either you use an explicit relative import. And the reader can see right way that you are using a local file.

Either you use an unambiguous absolute import. And the reader now that you use a package installed by pip.

tests is a super ambiguous absolute import! It actually only works for obscure non-deterministic reasons, and could totally fail if any package you installed contained a tests module...

Anna-Livia · 2018-05-14T15:25:50Z

tests/test_decomposition_fiches_de_paie.py

@@ -6,7 +6,8 @@
 import os

 from openfisca_core import decompositions
-from cache import tax_benefit_system
+from openfisca_core.commons import to_unicode
+from .cache import tax_benefit_system


Absolute imports :)

The test package is not under openfisca_france, so I don't see how I can use an absolute import.

Anna-Livia · 2018-05-14T15:26:04Z

tests/test_decompositions.py

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

 from openfisca_core import conv, decompositions, decompositionsxml

-from cache import tax_benefit_system
+from .cache import tax_benefit_system


Absolute imports :)

Anna-Livia · 2018-05-14T15:26:21Z

tests/test_parameters.py

@@ -3,7 +3,7 @@
 import datetime
 import json

-from cache import tax_benefit_system
+from .cache import tax_benefit_system


Absolute imports

Anna-Livia · 2018-05-14T15:26:31Z

tests/test_repairs.py

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

 from nose.tools import assert_equal

-from cache import tax_benefit_system
+from .cache import tax_benefit_system


Absolute imports

Anna-Livia · 2018-05-14T15:26:50Z

tests/test_suggestions.py

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

 from nose.tools import assert_equal

-from cache import tax_benefit_system
+from .cache import tax_benefit_system


Absolute imports

Anna-Livia · 2018-05-14T15:27:11Z

tests/test_tax_rates.py

@@ -1,7 +1,7 @@
 # -*- coding: utf-8 -*-

 from openfisca_core.rates import average_rate, marginal_rate
-from cache import tax_benefit_system
+from .cache import tax_benefit_system


Absolute imports :)

Anna-Livia · 2018-05-14T15:27:17Z

tests/utils.py

@@ -6,7 +6,7 @@
 from openfisca_core.tools import assert_near
 from nose.tools import nottest

-from cache import tax_benefit_system
+from .cache import tax_benefit_system


Absolute imports :)

fpagnoux · 2018-05-15T17:36:57Z

Tests are passing 🎉

fpagnoux · 2018-05-22T03:34:46Z

.circleci/config.yml

+
+      - run:
+          name: Run tests
+          command: |


This part differ from the common CircleCI configuration, as we use parallelism.

fpagnoux · 2018-05-22T03:34:57Z

.circleci/config.yml

+
+      - run:
+          name: Run tests
+          command: |


This part differ from the common CircleCI configuration, as we use parallelism.

fpagnoux · 2018-05-22T03:39:59Z

CircleCI conf adapted to fit the new consensus from OpenFisca-Country-Template.

PR ready for review :)

Anna-Livia · 2018-05-22T10:20:02Z

.circleci/config.yml

+# CircleCI 2.0 configuration file. See <https://circleci.com/docs/2.0/language-python/>.
+version: 2
+jobs:
+  build_python2:


Would it make it clearer to developpers if that section was called "test_python_2" ?
It seams to me that it would be more descriptive of what the script is doing.

I think build is a quite standard term for what we are doing here (installing the dependencies and running the tests).

Anna-Livia · 2018-05-22T10:21:12Z

.circleci/config.yml

+version: 2
+jobs:
+  build_python2:
+    parallelism: 3


Unless things have changed, we can use only 4 containers with the CircleCI free plan.
3 allows to speed up the tests in France, while still allowing fast non-parallel builds from core or country template to pass.

Adding a comment :)

Anna-Livia · 2018-05-22T15:15:25Z

.circleci/config.yml

+          name: Create a virtualenv
+          command: |
+            mkdir -p /tmp/venv/openfisca_france
+            virtualenv /tmp/venv/openfisca_france


Why create a virtualenv ? In my mental model, OpenFisca-France will be the only thing running in this Docker Instance. I'm curious of why we are going through the virtualenv steps ...

Short answer: whenever you are using python, virtualenvs tend to save you trouble.

In that particular case, Matti tried an implem without virtualenv. This implied chown-ing some repositories, which is much less standard than using a virtualenv.

Anna-Livia · 2018-05-22T15:21:40Z

.circleci/config.yml

+            - build_python3
+          filters:
+            branches:
+              only: master


When I look at the workflow on circle ci, it seems that only

- build_python2 - build_python3

are running and not

- deploy_python2 - deploy_python3

I don't thing it is due to the branch filtering because we had that in previous versions and the deployement phases did appear.

Pretty sure it's due to branch filtering. See here and here for Core examples.

Anna-Livia · 2018-05-22T15:23:55Z

.circleci/config.yml

+          command: |
+            mkdir -p /tmp/venv/openfisca_france
+            python -m venv /tmp/venv/openfisca_france
+            echo "source /tmp/venv/openfisca_france/bin/activate" >> $BASH_ENV


What I understant is that we are creating a $BASH_ENV environment variable.
I don't see where it is used...
I might be mistaken ...

We actually writing this command in a file which path is provided by the $BASH_ENV environment variable.

Assignment in bash would be BASH_ENV="some stuff"

Anna-Livia · 2018-05-22T15:33:41Z

.circleci/detect-functional-changes.sh

+
+last_tagged_commit=`git describe --tags --abbrev=0 --first-parent`  # --first-parent ensures we don't follow tags not published in master through an unlikely intermediary merge commit
+
+if git diff-index --name-only --exit-code $last_tagged_commit -- . `echo " $IGNORE_DIFF_ON" | sed 's/ / :(exclude)/g'`


I think a comment explaining what this if is doing might be useful.

Anna-Livia · 2018-05-22T15:41:03Z

.circleci/is-version-number-acceptable.sh

    git --no-pager log -1 $current_version
    echo
    echo "Update the version number in setup.py before merging this branch into master."
    echo "Look at the CONTRIBUTING.md file to learn how the version number should be updated."
    exit 1
 fi

-if git diff-index --quiet origin/master CHANGELOG.md
+if ! $(dirname "$BASH_SOURCE")/detect-functional-changes.sh | grep --quiet CHANGELOG.md


It is strange to me that ! detect-functional-changes.sh means that changed were actually detected, as I read !as "not"

Is the grep --quiet CHANGELOG.md applied in the git diff-index --name-only in detect-functional-changes.sh?

The command on the left of | returns the list of files that have changed.
The grep checks if the Changelog is among them.
The ! is here to because we want to raise an error if the changelog has not been updated.

Then shouldn't it be named detect-no-functional-changes.sh ?

So this line doesn't really care about the exit code of detect-functional-changes.sh. It's the grep exit code that is negated with !.

But I agree with you that that line and that line do not read well. Fixing that for all repos.

Anna-Livia · 2018-05-22T15:44:28Z

tests/test_basics.py

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

 import datetime

-from cache import tax_benefit_system
+from .cache import tax_benefit_system


tests.cache ?

Anna-Livia

I would like a bit more info on the circleci2 config before giving a go :)

fpagnoux requested review from Anna-Livia and sandcha May 12, 2018 23:48

fpagnoux commented May 12, 2018

View reviewed changes

Anna-Livia reviewed May 14, 2018

View reviewed changes

fpagnoux force-pushed the python3 branch 4 times, most recently from af71fa8 to 67c9f34 Compare May 15, 2018 17:26

fpagnoux requested a review from Anna-Livia May 15, 2018 17:36

fpagnoux force-pushed the python3 branch 4 times, most recently from 817e5bf to 03a3ff1 Compare May 22, 2018 03:32

fpagnoux added 13 commits May 21, 2018 23:32

Adapt code to python 3

aec4efe

Adapt CSV loading

51f0ed7

Calculate variable before using it

48fefa7

Use bytes for string arrays

0e994d5

Bump version number

a6db1c9

Use absolute imports

83c6e16

Remove test on the old API

b28e2c0

Adapt versement transport to Py3

078aa6d

Adapt scenarios to Py3

161d5fa

Adapt Zone APL loading to Python 3

50a1e10

Adapt to python 3

3018de4

Calculate variable before using it

84b912a

Start migrating to Circle v2

d36af9e

fpagnoux force-pushed the python3 branch from 03a3ff1 to e86b675 Compare May 22, 2018 03:33

fpagnoux commented May 22, 2018

View reviewed changes

Anna-Livia reviewed May 22, 2018

View reviewed changes

fpagnoux requested a review from Anna-Livia May 22, 2018 16:39

Anna-Livia approved these changes May 23, 2018

View reviewed changes

fpagnoux added 4 commits May 23, 2018 08:49

Adopt CircleCI conf from Country-Template

56932b1

Update Core dependency

a48b87f

Deploy web API

204ad47

Make exit code more semantic

c040591

fpagnoux force-pushed the python3 branch from 7309dae to c040591 Compare May 23, 2018 12:49

fpagnoux merged commit ee86059 into master May 23, 2018

fpagnoux deleted the python3 branch May 23, 2018 15:24

bonjourmauko added the kind:solution label Jul 23, 2018

		@@ -1,5 +1,11 @@
		# Changelog

		## 21.1.0 [#980](https://github.com/openfisca/openfisca-france/pull/980)


		last_tagged_commit=`git describe --tags --abbrev=0 --first-parent` # --first-parent ensures we don't follow tags not published in master through an unlikely intermediary merge commit

		if git diff-index --name-only --exit-code $last_tagged_commit -- . `echo " $IGNORE_DIFF_ON" \| sed 's/ / :(exclude)/g'`

Adapte à Python 3 #980

Adapte à Python 3 #980

Conversation

fpagnoux commented May 12, 2018 • edited by bonjourmauko Loading

fpagnoux May 12, 2018 • edited Loading

Choose a reason for hiding this comment

Anna-Livia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpagnoux May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpagnoux May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpagnoux commented May 14, 2018

Anna-Livia commented May 14, 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

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

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 May 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpagnoux commented May 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpagnoux May 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpagnoux May 22, 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

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

fpagnoux May 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anna-Livia left a comment

Choose a reason for hiding this comment

fpagnoux commented May 12, 2018 •

edited by bonjourmauko

Loading

fpagnoux May 12, 2018 •

edited

Loading

fpagnoux May 14, 2018 •

edited

Loading

fpagnoux May 14, 2018 •

edited

Loading

fpagnoux May 22, 2018 •

edited

Loading

fpagnoux May 22, 2018 •

edited

Loading

fpagnoux May 23, 2018 •

edited

Loading