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

Fix conda upload #2374

Merged
merged 15 commits into from
Oct 31, 2024
Merged

Fix conda upload #2374

merged 15 commits into from
Oct 31, 2024

Conversation

benoit-cty
Copy link
Contributor

@benoit-cty benoit-cty commented Oct 23, 2024

  • Correction d'un crash.
  • Périodes concernées : aucune.
  • Zones impactées : workflow.yml.
  • Détails :
    • La publication conda ne fonctionnait plus suite au passage à pyproject.toml : C:\hostedtoolcache\windows\Python\3.9.13\x64\python3.exe: can't open file 'D:\a\openfisca-france\openfisca-france\setup.py': [Errno 2] No such file or directory

Fait suite à #2369 dans laquelle il y avait des manques sur Conda. La CI a donc été mise à jour pour refléter openfisca/country-template#157

Ces changements :

  • Corrigent ou améliorent un calcul déjà existant.

  • Passage de build-conda-${{ hashFiles('.conda/recipe.yaml') }}-${{ github.sha }} à build-conda-${{ hashFiles('pyproject.toml') }}-${{ github.sha }} car c'est lui qui contient vraiment la version et les dépendances.

  • Passage de toutes les étapes conda sur un runner Windows pour valider que cela fonctionne correctement et éviter des problèmes de chemin, encoding, fin de ligne...

  • Retrait de l'artifact qui n'était plus utilisé

  • Ajout d'un commentaire dans check_path_length.py pour indiquer que le script est utilisé en CI.

  • J'ai modifié temporairement la CI pour livrer le package en dehors de master et ça a fonctionné : https://anaconda.org/openfisca/openfisca-france Voici les logs de la CI : log


Quelques conseils à prendre en compte :

@benoit-cty benoit-cty marked this pull request as ready for review October 23, 2024 13:32
@benoit-cty benoit-cty changed the title Draft: fix conda upload Fix conda upload Oct 23, 2024
@benoit-cty benoit-cty requested a review from sandcha October 23, 2024 17:29
restore-keys: |
build-conda-${{ hashFiles('.conda/recipe.yaml') }}
build-conda-
key: build-conda-${{ github.sha }}
Copy link
Contributor

Choose a reason for hiding this comment

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

La liste des dépendances n'est-elle pas encore utile à distinguer les caches ? Le fichier recipe.yaml existe encore (malgré le passage à pyproject.toml).

Copy link
Contributor

@sandcha sandcha Oct 31, 2024

Choose a reason for hiding this comment

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

Vu ensemble : ici, on retirait également restore-keys pensant se prémunir de situations où on récupérerait un ancien cache sur des dépendances ayant évolué (cas déjà rencontré dans le passé hors conda). Mais, nous avions mal interprété cette fonction qui s'adresse à d'autres fichiers que ceux référencés dans key. Bilan : en le supprimant, on retire une configuration inutile mais on ne se prémunit pas de l'usage d'un vieux cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

J'ai dû passer de build-conda-${{ hashFiles('.conda/recipe.yaml') }}-${{ github.sha }} à build-conda-${{ github.sha }} pour les clefs de cache car sinon il était impossible de récupérer le cache sous Windows : le hash n'était pas le même. Sûrement à cause d'un problème d'encoding ou fin de ligne.

Suite à discussion, j'ai passé toutes les étapes Conda sur des runners Windows pour mettre le hashFiles de pyproject.toml comme c'est lui qui connait la version et les dépendances.

.github/workflows/workflow.yml Show resolved Hide resolved
.github/workflows/workflow.yml Show resolved Hide resolved
path: /tmp/conda-bld
key: build-conda-${{ github.sha }}
fail-on-cache-miss: true
enableCrossOsArchive: true # To allow restore on windows
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

.github/workflows/workflow.yml Outdated Show resolved Hide resolved
- uses: actions/checkout@v4
activate-environment: openfisca
miniforge-version: latest
python-version: 3.9.12
Copy link
Contributor

Choose a reason for hiding this comment

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

Utiliser la version Python la plus récente testée ? 3.10.6 ? 🤔

Ceci dépasserait la résolution pure de l'issue visée par cette PR et aurait un impact ailleurs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fait.

.github/workflows/workflow.yml Show resolved Hide resolved
run: mamba install --channel file:///tmp/conda-bld --channel openfisca openfisca-france
- name: Test conda package
shell: bash -l {0}
run: openfisca test tests/formulas/irpp.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

Pourquoi réduisons-nous à un seul test ? Dans le passé, les tests permettaient de relever les erreurs de chemins de paramètres trop longs sous Windows qui pouvaient survenir à n'importe quel endroit modifié par une PR. Considérons-nous que Test max path length suffit ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

C'est un problème de temps de CI : tout tester prend 1 heure. On pourrait aussi mettre des tests sous forme de matrice. Cependant vu que nous ne savons pas s'il y a des utilisateurs des paquets conda, il ne semble pas nécessaire de gêner les contributeurs avec des tests supplémentaires.

Copy link
Contributor

Choose a reason for hiding this comment

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

Bilan, pour détecter les erreurs sur Windows de :

  • paramètres : le script Test max path length (du jobtest-on-windows à ne pas confondre avec le job test-path-length exécuté sur ubuntu) vérifie le fait de ne pas dépasser la limite par défaut de longueur de path
  • variables, chargement du TaxBenefitSystem et bonne installation de la commande openfisca : l'exécution d'un seul test YAML suffit et est économe en temps de calcul ; ici on teste déjà tout ce qu'il y a dans irpp.yaml (pas d'erreur attendue/rencontrée spécifique à des syntaxes de règles python)

.github/workflows/workflow.yml Show resolved Hide resolved
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 tout de fix de configuration !
Validé avec 3 mini points à vérifier avant merge :

  • un slash en dernier commentaire qui devrait peut-être remplacer un backslash
  • aligner les versions de python sur windows à 3.10.6
  • et un commentaire de todo à retirer probablement

@@ -19,7 +19,7 @@ source:
build:
noarch: python
number: 0
script: "pip install . -v"
script: "pip install ."
Copy link
Contributor

Choose a reason for hiding this comment

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

Permet d'éviter un job rouge en sortie de CI pour log trop long.

build-conda-${{ hashFiles('.conda/recipe.yaml') }}
build-conda-
path: ${{ runner.temp }}\conda-bld
key: build-conda-${{ hashFiles('pyproject.toml') }}-${{ github.sha }}
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️ Pour la source unique qu'est pyproject.toml maniement pour l'information sur les dépendances.

run: mamba install --channel file:///tmp/conda-bld --channel openfisca openfisca-france
- name: Test conda package
shell: bash -l {0}
run: openfisca test tests/formulas/irpp.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

Bilan, pour détecter les erreurs sur Windows de :

  • paramètres : le script Test max path length (du jobtest-on-windows à ne pas confondre avec le job test-path-length exécuté sur ubuntu) vérifie le fait de ne pas dépasser la limite par défaut de longueur de path
  • variables, chargement du TaxBenefitSystem et bonne installation de la commande openfisca : l'exécution d'un seul test YAML suffit et est économe en temps de calcul ; ici on teste déjà tout ce qu'il y a dans irpp.yaml (pas d'erreur attendue/rencontrée spécifique à des syntaxes de règles python)

.github/workflows/workflow.yml Outdated Show resolved Hide resolved
.github/workflows/workflow.yml Outdated Show resolved Hide resolved
conda config --set anaconda_upload yes
conda build --channel conda-forge --channel openfisca --token ${{ secrets.ANACONDA_TOKEN }} --user openfisca .conda
conda install --yes anaconda-client
anaconda -t ${{ secrets.ANACONDA_TOKEN }} upload --user openfisca ${{ runner.temp }}\conda-bld/noarch/openfisca-france-*
Copy link
Contributor

Choose a reason for hiding this comment

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

Slash à vérifier ici ? : ${{ runner.temp }}\conda-bld/noarch/openfisca-france-*
À remplacer par ceci ? : ${{ runner.temp }}/conda-bld/noarch/openfisca-france-*

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merci, c'est changé. Ca fonctionnait quand même car Windows supporte maintenant les deux écritures.

openfisca_france/scripts/check_path_length.py Show resolved Hide resolved
benoit-cty and others added 13 commits October 31, 2024 19:59
Checkout needed to get github.sha

ci : use-mamba

ci : use-mamba

wip: try to understand why cache key changes

use only github.sha

enableCrossOsArchive

remove ${{ hashFiles('.conda/recipe.yaml')

Add mamba to publish

Fix conda publish
Co-authored-by: sandcha <[email protected]>
Co-authored-by: sandcha <[email protected]>
Co-authored-by: sandcha <[email protected]>
@benoit-cty benoit-cty enabled auto-merge October 31, 2024 19:05
@benoit-cty benoit-cty merged commit 0639cfa into master Oct 31, 2024
38 checks passed
@benoit-cty benoit-cty deleted the fix_conda_master_ci branch October 31, 2024 19:13
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.

2 participants