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

Docker #192

Closed
wants to merge 15 commits into from
Closed

Docker #192

wants to merge 15 commits into from

Conversation

benoit-cty
Copy link
Contributor

@benoit-cty benoit-cty commented Mar 18, 2021

Bonjour !

I'm from the Leximpact team, I submit this PR as I thing it will help others to easily process ERFS FPR files.

New features

  • Use Docker to install everything that is needed for ERFS FPR and process files.
  • Allow input_data_builder to read year from raw_data.ini like survey-manager

Technical changes

  • Small addition in .gitignore to prevent commit of secret files (*.sas7bdat and *.h5) and the json generated by build-collection
  • All others changes are in docker/erfs folder. With a README in it.
  • Tested on Ubuntu 20.04 with Docker version 19.03.13, build cd8016b6bc

Note to reviewers

You have to use a specific command to run the script:

docker run --rm -v $PWD/docker/data:/opt/data \
 -v $PWD/openfisca_france_data:/opt/openfisca-france-data/openfisca_france_data \
 -v $PWD/docker:/opt/openfisca-france-data/docker \
 openfisca-france-data \
 /opt/openfisca-france-data/docker/erfs-fpr.sh

The "-v $PWD:/opt/openfisca-france-data" parameter mount you local version of OpenFisca to replace the one from OpenFisca master inside the container.

@benoit-cty
Copy link
Contributor Author

@benjello Bonjour, la CI ne fonctionne pas car il n'arrive pas à récupérer le code, ceci ne semble pas avoir de lien avec la PR, que me conseille-tu ?
CC : @sandcha

@benjello
Copy link
Member

@benoit-cty ; franchement je ne sais pas ce qui ne va pas mais ce n'est pas bloquant pour moi. Donc on peut merger.
Faut juste updater la version.

@benoit-cty
Copy link
Contributor Author

Merci @benjello , je viens de push le bump de version.

@benjello
Copy link
Member

@sandcha @maukoquiroga si vous comprenez pour circleci râle alors qu'il y a deux c'est passé sans problème je suis preneur.

@benoit-cty
Copy link
Contributor Author

@sandcha @maukoquiroga si vous comprenez pour circleci râle alors qu'il y a deux c'est passé sans problème je suis preneur.

Il y a bien une clef SSH de configuré sur Circle CI mais est-elle encore bien présente dans la config de Github ?

Côté CI il y a :
openfisca/openfisca-france-data user key : ed:78:07:57:23:2b:40:98:47:d9:74:ed:59:85:d0:82

@benjello benjello mentioned this pull request Mar 30, 2021
@sandcha
Copy link
Contributor

sandcha commented Apr 13, 2021

Sauf avis contraire, je tenterais une régénération de clef SSH entre CircleCI et GitHub pour tenter de débloquer le checkout. Ok pour toi @benjello ?

@sandcha
Copy link
Contributor

sandcha commented Apr 24, 2021

@benjello @benoit-cty Le bug sur le job Checkout code de l'intégration continue a été résolu.
J'ai ajouté la clef SSH de déploiement CircleCI à ce dépôt (elle est ici).

Plus d'information sur cette résolution à venir.

@benjello
Copy link
Member

Merci @sandcha. Reste à résoudre openfisca/openfisca-core#1010.

@benoit-cty
Copy link
Contributor Author

openfisca/openfisca-core#1010 is now solved, what I have to do now?
Upgrade openfisca core in open fisca France data ?

Copy link
Contributor

@sandcha sandcha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merci @benoit-cty pour cette documentation Docker 🙌
J'ai malheureusement eu une erreur bloquante au docker run. Manquerait-il une étape au README ? 🤔
Si ce n'est pas le cas, je te propose a minima d'indiquer que la configuration actuelle nécessiterait d'être sous Linux.

docker/erfs/README.md Outdated Show resolved Hide resolved
docker/erfs/README.md Outdated Show resolved Hide resolved
docker/erfs/README.md Outdated Show resolved Hide resolved

```sh
cd openfisca-france-data/docker/erfs
docker build -t openfisca-france-data-erfs --build-arg USER_ID=$(id -u) --build-arg GROUP_ID=$(id -g) .
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nécessite le docker daemon. Ajouter une ligne pour dire que l'application Docker doit être active ? 😊
Sinon, quand on a osé l'éteindre (quelle folie !) on obtient le message suivant :

Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?

Copy link
Contributor

@sandcha sandcha May 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

J'ai obtenu cette erreur sous macOS pour l'étape docker build :

...
 => ERROR [ 2/17] RUN addgroup --gid 20 user                                                                                         0.5s
------                                                                                                                                    
 > [ 2/17] RUN addgroup --gid 20 user:
#5 0.331 addgroup: The GID `20' is already in use.
------
executor failed running [/bin/sh -c addgroup --gid $GROUP_ID user]: exit code: 1

Et elle semble être relative à l'OS d'après ce commentaire.
À minima, indiquer que ces commandes ont été réalisées sous Linux ?

Sinon, indiquer les valeurs d'ID et ne pas se référer à la commande id ?
Ceci a fonctionné pour moi mais n'est peut-être pas conseillé 🤔 (je n'en connais pas les effets de bord) :

docker build -t openfisca-france-data-erfs --build-arg USER_ID=1000 --build-arg GROUP_ID=1000 .

L'idée est venue de ce post évoquant les valeurs Linux/macOS et a été confirmée par cette page qui évoque les valeurs sous Linux.

Ceci sachant que sous macOs, les valeurs d'id sont les suivantes :

$ id
uid=501(sacha) gid=20(staff) groups=20(staff),12(everyone),61(localaccounts),79(_appserverusr),80(admin),81(_appserveradm),98(_lpadmin),701(com.apple.sharepoint.group.1),33(_appstore),100(_lpoperator),204(_developer),250(_analyticsusers),395(com.apple.access_ftp),398(com.apple.access_screensharing),399(com.apple.access_ssh),400(com.apple.access_remote_ae)

docker/erfs/README.md Outdated Show resolved Hide resolved
docker/erfs/README.md Outdated Show resolved Hide resolved
```sh
cd openfisca-france-data/docker/erfs
docker build -t openfisca-france-data-erfs --build-arg USER_ID=$(id -u) --build-arg GROUP_ID=$(id -g) .
docker run --rm -v $PWD/data:/opt/erfs/data openfisca-france-data-erfs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

J'obtiens l'erreur suivante à l'exécution du docker run 😢 :

...
Generate a flattened file consumable by openfisca...
/opt/erfs/entrypoint.sh: line 15:    13 Killed                  build-collection -c erfs_fpr -d -m -v -p $DATA_FOLDER
Traceback (most recent call last):
  File "/opt/openfisca-france-data/openfisca_france_data/erfs_fpr/input_data_builder/__init__.py", line 68, in <module>
    build(year = year, export_flattened_df_filepath = export_flattened_df_filepath)
  File "/opt/openfisca-france-data/openfisca_france_data/erfs_fpr/input_data_builder/__init__.py", line 32, in build
    preprocessing.build_merged_dataframes(year = year)
  File "/home/user/.local/lib/python3.7/site-packages/openfisca_survey_manager/temporary.py", line 38, in func_wrapper
    return func(*args, temporary_store = temporary_store, **kwargs)
  File "/opt/openfisca-france-data/openfisca_france_data/erfs_fpr/input_data_builder/step_01_preprocessing.py", line 30, in build_merged_dataframes
    eec_menage = survey.get_values(table = f"fpr_mrf{yr}e{yr}t4", ignorecase=True)
  File "/home/user/.local/lib/python3.7/site-packages/openfisca_survey_manager/surveys.py", line 182, in get_values
    assert self.hdf5_file_path is not None
AssertionError
mv: cannot stat '/opt/erfs/data/erfs_flat_*.h5': No such file or directory

Qu'est-ce que j'ai fait pour en arriver là ?

Dans un environnement virtuel neuf, j'ai exécuté, make install, make test puis les étapes de ce README.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avec @benoit-cty, nous avons épuisé nos premières pistes et serions intéressés par le test sur une autre machine. À tout hasard @benjello aurais-tu docker d'installé chez toi ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On avait une piste sur le compte utilisateur qui n'aurait pas .config/openfisca-survey-manager mais ce n'était pas ça.

Le fait est que self.hdf5_file_path est à None dans Survey et qu'on ne comprend pas pourquoi.

Je pense qu'il faudrait des vérifications dans survey manager pour voir où est l'erreur.

@benoit-cty
Copy link
Contributor Author

Bonjour @benjello j'ai revu pas mal de choses, pourrais-tu tester de ton côté ?
La doc est là https://github.com/openfisca/openfisca-france-data/blob/docker/docker/README.md

Par contre, comme on est dans une branche, pour lancer le traitement il faut utiliser:

docker run --rm -v $PWD/docker/data:/opt/data \
 -v $PWD/openfisca_france_data:/opt/openfisca-france-data/openfisca_france_data \
 -v $PWD/docker:/opt/openfisca-france-data/docker \
 openfisca-france-data \
 /opt/openfisca-france-data/docker/erfs-fpr.sh

Ce qui monte le code local dans le conteneur.

@benoit-cty benoit-cty removed the request for review from magemax May 11, 2021 07:53
@benjello
Copy link
Member

@benoit-cty : je n'ai jamais utilisé docker donc je ne sais pas si je suis la bonne personne pour tester cela ...

@benjello benjello closed this Jun 7, 2021
@benjello
Copy link
Member

benjello commented Jun 7, 2021

@benoit-cty : j'ai fermé la PR mais puis-je aussi virer la branche sachant que j'ai intégré ce qui était sur leximpact

@benoit-cty benoit-cty deleted the docker branch June 7, 2021 11:17
@benoit-cty
Copy link
Contributor Author

@benjello : Oui, je viens d'effacer la branche.

@sandcha sandcha mentioned this pull request Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants