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

Add stream parsing in Schema class #25

Merged
merged 1 commit into from
Apr 22, 2024

Conversation

antoine-b-smartway
Copy link
Contributor

Le but de cette PR est d'ajouter un moyen de lire séquentiellement les lignes parsées d'un fichier au lieu de tout lire d'un coup pour réduire l'empreinte mémoire d'un import de fichier.

Des tests temporaires ont été mis pour vérifier:

  • que c'est bien séquentiel
  • que la consommation de mémoire reste stable avec cette nouvelle méthode

J'ai laissé ces tests pour la PR mais je vais les enlever avant le merge:

  • le test de mémoire est long et compliqué de mettre des assertions fiables dessus
  • le test d'ordre n'apporte pas grand chose, ça revient à tester Iterable en python (et en plus je ne vois pas comment l'écrire sans variable statique)

pyproject.toml Outdated
@@ -33,3 +33,4 @@ exclude = [".git/", ".pytest_cache/", ".venv"]

[tool.pytest.ini_options]
python_files = ["tests/*"]
log_cli = true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nécessaire pour les tests temporaires pour afficher des logs dans le résultat des tests exécutés avec pytest. Sera enlevé en même temps que les tests.

@antoine-b-smartway antoine-b-smartway self-assigned this Apr 17, 2024
@antoine-b-smartway antoine-b-smartway requested a review from a team April 17, 2024 08:49
README.md Outdated Show resolved Hide resolved

return items, errors

def stream_parse(self, data: Union[bytes, BytesIO]) -> Iterable[Tuple[dict, dict]]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Pour info, il n'est plus nécessaire d’hériter des types de typing pour les collections, et il existe une syntaxe dédiée à l'union). le type dict n'est pas non plus complet, mais ça devient un poil complexe au niveau refacto, on peut utiliser Any pour la valeur, la clef par contre devrait tjrs etre des str, non ?
On pourrait réécrire le type en :

def stream_parse(self, data: bytes | BytesIO) -> Iterable[tuple[dict[str, Any], dict[str, Any]]]:
    ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Je suis d'accord avec toi Mathias !
Mais pour faire ça il faut d'abord bump la version de Python du projet dans le Dockerfile (passer de 3.9 à 3.11) et faire pareil dans pyproject.toml.
C'est pas un gros changement et de toute façon le seul service qui consomme magicparse est external-is qui est déjà en Python 3.11

Copy link
Contributor

@ducdetronquito ducdetronquito Apr 17, 2024

Choose a reason for hiding this comment

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

On peut faire ça dans une autre PR en parallèle aussi, c'est pas directement lié à celle ci et ce n'est pas bloquant.

Copy link
Contributor

@ducdetronquito ducdetronquito left a comment

Choose a reason for hiding this comment

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

Quelques remarques mais sinon c'est top !

@ducdetronquito
Copy link
Contributor

Faudrait que tu vois avec un gars de ton équipe pour avoir poetry fonctionnel sur ta machine + les hooks de pre commit!
Ca t'éviterait les erreurs en CI ^^

tests/test_schema.py Outdated Show resolved Hide resolved

class TestStreaming(TestCase):

def test_stream_parse_errors_do_not_halt_parsing(self):
Copy link
Member

Choose a reason for hiding this comment

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

J'ai l'impression que tu tests beaucoup de chose en un :)

Quel est l'intention derrière ce test spécifiquement ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oui, J'ai repris un test existant sur parse qui me semblait testé un cas passant et un cas échouant.
Pour le reste je me suis dit que dupliquer tous les tests n'avait pas de valeur ajoutée sachant que parse appelle stream_parse et me semblait déjà bien couverte.

Copy link
Member

@sGeeK44 sGeeK44 left a comment

Choose a reason for hiding this comment

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

Pas grand chose à dire de plus que les autres ! :)

@antoine-b-smartway
Copy link
Contributor Author

Faudrait que tu vois avec un gars de ton équipe pour avoir poetry fonctionnel sur ta machine + les hooks de pre commit! Ca t'éviterait les erreurs en CI ^^

Une doc que je pourrais suivre pour installer ça ?

@antoine-b-smartway antoine-b-smartway force-pushed the task/int-699-stream-parsing branch 2 times, most recently from 35815c3 to 3fc9254 Compare April 18, 2024 14:42
Copy link
Contributor

@ducdetronquito ducdetronquito left a comment

Choose a reason for hiding this comment

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

Faut juste un passage du formateur black + flake8 et ça devrait passer la CI :)

@antoine-b-smartway antoine-b-smartway force-pushed the task/int-699-stream-parsing branch from fb32e77 to e11a82d Compare April 19, 2024 14:38
@ducdetronquito
Copy link
Contributor

Pour créer une nouvelle version il te faut:

  • merger cette PR dans la branche main
  • Créer un tag sur la branch main 0.12.0 (git tag -a 0.12.0)
  • Push (la CI va s'occuper de créer une "release github" + déployer sur CodeArtifact) (git push --tags)

@antoine-b-smartway antoine-b-smartway merged commit 6c81c2a into main Apr 22, 2024
5 checks passed
@antoine-b-smartway antoine-b-smartway deleted the task/int-699-stream-parsing branch April 22, 2024 14:58
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.

4 participants