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

Refactor export vecteur #386

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

nlenglet-ign
Copy link
Collaborator

Refonte de l'export vecteur du graphe :

  • un seul script permet maintenant de faire toutes les étapes préalables avant distribution du calcul
  • mise à jour des tests
  • mise à jour de la documentation

@coveralls
Copy link

coveralls commented Mar 3, 2023

Pull Request Test Coverage Report for Build 4324283717

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 95.961%

Totals Coverage Status
Change from base Build 4172909642: 0.0%
Covered Lines: 5935
Relevant Lines: 6137

💛 - Coveralls

Copy link
Collaborator

@amrosu amrosu left a comment

Choose a reason for hiding this comment

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

Plusieurs corrections sont nécessaires :

Comment on lines 27 to 29
parser.add_argument("-b", "--branch",
help="id of branch of cache to use as source for patches (default: None)",
default=None)
Copy link
Collaborator

@amrosu amrosu Mar 9, 2023

Choose a reason for hiding this comment

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

Le param '--branch' n'est plus optionnel, default=None n'est plus géré - gérer ce cas ou rendre le param obligatoire

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Corrigé, le paramètre '--branch' est maintenant obligatoire.

Comment on lines 53 to 56
try:
os.mkdir(args.output)
except FileExistsError:
print("ERREUR : Le dossier de sortie existe déjà.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Erreur ou alerte ? le comportement est celui d'une alerte, mais on affiche un message d'erreur

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Message d'alerte modifié

Comment on lines 64 to 68
if cache['id'] == args.cache:
cache_path = cache['path']
break
else:
cache_path = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

"else" pas nécessaire : initialiser cache_path = None
Et faire plutôt un "raise SystemExit" à la place de "SystemError", car sortie plus propre

Copy link
Collaborator Author

@nlenglet-ign nlenglet-ign Mar 14, 2023

Choose a reason for hiding this comment

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

Corrigé, plus de 'else' et utilisation d'un 'SystemExit'

Comment on lines 69 to 96
def check_branch_patch(branch, id_branch_patch):
if branch and id_branch_patch and int(branch) != id_branch_patch:
raise SystemExit(f"** ERREUR: "
f"Pas de correspondance entre la branche indiquée '{branch}' "
f"et celle des retouches '{id_branch_patch}' !")

if branch and not id_branch_patch:
raise SystemExit(f"** ERREUR: "
f"Branche de retouches indiquée '{branch}', mais aucune retouche !")

if not branch and id_branch_patch:
print(f"** La branche de retouches traitée est : '{id_branch_patch}'")
branch = str(id_branch_patch)
Copy link
Collaborator

@amrosu amrosu Mar 9, 2023

Choose a reason for hiding this comment

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

J'avais mis en place ces vérifications pour le fonctionnement d'avant, avec --branche default=None pour gérer aussi le cas de la branche 'orig' ou branche sans saisie.
Cela n'est plus fonctionnel - à faire

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

De nouveau fonctionnel avec les modifications apportées

Comment on lines 53 to 55
try:
os.mkdir(args.output)
except FileExistsError:
Copy link
Collaborator

Choose a reason for hiding this comment

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

À faire après les verif car pas utile de créer un dossier si on ne met rien dedans (comme dans le cas de sortie à cause d'erreur)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Code déplacé après les vérifications préalables au calcul

# TODO : id cache puis on recupere le folder

parser = argparse.ArgumentParser()
parser.add_argument("-c", "--cache", required=True, type=int, help="id of input cache")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Demander plutôt le chemin vers le cache pour assurer une compatibilité avec le chemin de l'utilisateur

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Retour à l'utilisation du chemin vers le cache plutôt que l'id

Copy link
Collaborator

@amrosu amrosu left a comment

Choose a reason for hiding this comment

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

Nettoyage de code pas fait - à faire

Comment on lines 52 to 55
# verifier qu'un cache correspondant a l'id existe bien et recuperer son path
req_get_caches = f'{args.url}/caches'
resp_get_caches = check_get_post(req_get_caches)
list_caches = response2pyobj(resp_get_caches)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Un exemple de code à nettoyer : ces lignes sont obsolètes, pas utiles après le remplacement de l'id cache pour le param --cache avec son chemin.

Il y a d'autres endroits avec du code pas utile / utilisé, obsolète etc. à nettoyer

Copy link
Collaborator

@amrosu amrosu left a comment

Choose a reason for hiding this comment

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

En attente de résultats de tests de l'utilisateur principal en condition de production pour valider les corrections et évolutions demandées sur l'export vecteur du graphe

@amrosu amrosu added the wip Travail en cours (ne pas merger) label Apr 18, 2023
@amrosu amrosu linked an issue Oct 5, 2023 that may be closed by this pull request
6 tasks
refactor(export): move scripts to subdir

refactor(export): new version with mtd join

feat(cache): add export_graph script

fix(export): add init files

fix(export): import modification

fix(export): flake exceptions

refactor(export): v0 export avec prep

refactor(export): v1 export fonctionnel, tests complets a faire

fix(export): flake deprecated regex

refactor(export): use cache id instead of cache path

cleaning

clean
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wip Travail en cours (ne pas merger)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Export vecteur : reste à faire
3 participants