Skip to content

feat: add Hugging Face dataset pull/push provider#36

Open
kaaloo wants to merge 2 commits into
mainfrom
feat/hf-dataset-provider
Open

feat: add Hugging Face dataset pull/push provider#36
kaaloo wants to merge 2 commits into
mainfrom
feat/hf-dataset-provider

Conversation

@kaaloo

@kaaloo kaaloo commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Summary

  • new eval-transcript huggingface dataset {ls,pull,push} subcommands for sharing the benchmark corpus on a single HF Dataset repo
  • HuggingFaceClient with token-aware constructor, public-repo push guard, and idempotent pull
  • huggingface-hub added to core dependencies
  • .env.example documents HF_TOKEN / HF_ORG / HF_DATASET_REPO
  • README section with 5-line usage example
  • unit tests cover auth, public/private guard, idempotent pull, push, and CLI dispatch

Closes #28

Verification

  • uv run python -m compileall -q src tests
  • uv run python -m unittest discover -s tests -v
  • uv run pre-commit run --all-files

👾 Generated with Letta Code

Read and write the benchmark corpus from a single HF Dataset repo with
audio + ground_truth files. Writes require HF_TOKEN and refuse to push
to a public dataset repo. The provider is read/write only; ASR
inference stays with the existing oMLX, Albert, Scaleway, and
ElevenLabs providers.

Closes #28

👾 Generated with [Letta Code](https://letta.com)

Co-Authored-By: Letta Code <[email protected]>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: be7c963f86

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/eval_transcript/huggingface.py Outdated
Comment thread src/eval_transcript/huggingface.py Outdated

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces Hugging Face Hub integration to manage the benchmark corpus, adding CLI commands to list, pull, and push datasets. Feedback on the implementation focuses on optimizing file uploads by batching them into a single atomic commit using create_commit rather than uploading files individually, and updating the associated protocol and test mocks. Additionally, the reviewer suggests improving memory efficiency during downloads by streaming files with shutil.copy, catching the specific RepositoryNotFoundError instead of a generic exception, simplifying path parsing with pathlib.Path, and removing the unused file_sha256 helper function.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/eval_transcript/huggingface.py Outdated
Comment thread src/eval_transcript/huggingface.py Outdated
Comment thread src/eval_transcript/huggingface.py Outdated
Comment thread src/eval_transcript/huggingface.py
Comment thread src/eval_transcript/huggingface.py Outdated
Comment thread src/eval_transcript/huggingface.py Outdated
Comment thread tests/test_huggingface.py Outdated
Module-level hf_hub_download, corpus-folder scope for sample discovery,
single atomic create_commit push, shutil.copy streaming download, and
RepositoryNotFoundError-only missing-repo branch. Remove the unused
file_sha256 helper.

👾 Generated with [Letta Code](https://letta.com)

Co-Authored-By: Letta Code <[email protected]>
@kaaloo
kaaloo requested a review from benoitvx June 11, 2026 15:00
@benoitvx

Copy link
Copy Markdown
Contributor

@kaaloo j'ai relu en détail l'état post-fb5aeba (donc après les retours gemini/codex, tous bien intégrés : download module-level, commit atomique unique, scoping audio/+ground_truth/, RepositoryNotFoundError ciblé, shutil.copy, Protocol à jour). Le design est propre — HfApiLike pour tester sans dépendre du vrai hub, push privé-par-défaut avec garde anti-public, pull idempotent. Quelques points que je n'ai pas vus remontés ailleurs :

🟠 P1 — --samples ne strippe pas les espaces

Dans le dispatch CLI : [item for item in (args.samples or "").split(",") if item]. Avec --samples "sample-a, sample-b", le 2ᵉ id vaut " sample-b" (espace initial) → il ne matche pas available → erreur Samples not found: sample-b trompeuse. Un .strip() par item règle ça :

sample_ids = [s.strip() for s in args.samples.split(",") if s.strip()] if args.samples else None

🟡 P2 — sélection du ground truth : la priorité .md > .txt documentée n'est pas respectée

manifest.py définit GROUND_TRUTH_SUFFIXES = (".md", ".txt") comme un tuple ordonné (« a sample with both extensions resolves to the first match » → .md gagne). Côté HF, _candidate_paths_for_field renvoie les matches dans l'ordre de list_repo_files, et _write_one prend candidates[0]. Donc pour un sample ayant ground_truth/<id>.md et .txt, le fichier pull dépend de l'ordre de listing du hub, pas de la priorité documentée → un pull peut récupérer le .txt là où score/manifest utiliseraient le .md. Idem (sans priorité définie) pour un sample avec .mp3 + .wav côté audio : candidates[0] est non déterministe. Suggestion : trier les candidats selon l'ordre de priorité de manifest plutôt que l'ordre de listing.

🟡 P2 — constantes d'extensions dupliquées (risque de dérive)

AUDIO_FILE_EXTENSIONS et GROUND_TRUTH_FILE_EXTENSIONS recopient à l'identique manifest.AUDIO_SUFFIXES et manifest.GROUND_TRUTH_SUFFIXES. Le jour où on ajoute un format dans manifest (ou où on change la priorité GT), le provider HF divergera silencieusement (un format reconnu par le scoring ne serait ni pull ni push). Réutiliser les constantes de manifest supprime le risque et règle aussi le P2 ci-dessus d'un coup.

🟢 P3 — défensif / UX

  • Garde anti-public fail-open : is_private = bool(getattr(info, "private", True)) ⇒ si l'attribut private était absent/renommé côté DatasetInfo, on retombe sur True (= privé) et on pousse quand même. Pour une garde de sécurité, je ferais plutôt fail-closed (refuser/lever si la confidentialité ne peut pas être confirmée). Risque réel faible (l'attribut est fiable aujourd'hui), mais c'est la seule barrière contre un push public.
  • ls/pull en require_token=False : comme push impose le privé, lister/puller ce même repo sans token renverra un 401 HF brut plutôt que le message clair « HF_TOKEN required ». OK pour des corpus publics, mais sur le corpus privé par défaut, un petit hint serait plus sympa.
  • push est additif seulement : un sample supprimé en local reste sur le hub (pas de prune). C'est sans doute voulu (add ≠ mirror), mais ça mériterait une ligne dans le README/--help.
  • Cosmétique : le --help de --repo affiche defaults to $eval-transcript-corpus (le f"${HF_DEFAULT_DATASET_REPO}" interpole la valeur, d'où le $ parasite). Idem le double-pull laisse une copie dans le cache HF + une dans data/ (hf_hub_download(..., local_dir=...) éviterait la duplication).

✅ Tests

Couverture solide (auth, garde public/privé, pull idempotent, commit atomique unique, dispatch CLI). Si tu touches au P1/P2, deux cas à ajouter : --samples " a , b " (strip) et un sample ayant .md et .txt (priorité).

Rien de bloquant à mes yeux hormis le P1 (vite réglé). Beau travail sur la testabilité 👍

@benoitvx benoitvx left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review (Benoit) : super base, mais une décision de design à acter avant merge

Merci Luis, le provider est propre et bien testé. Avant de merger, un point de stratégie produit qu'on a tranché côté éval cette semaine et qui inverse une hypothèse de la PR.

Décision actée côté éval

On utilisera Hugging Face pour de la donnée publique uniquement : le corpus des discours officiels (déjà en ligne sur info.gouv.fr, référence lissée), dans un dataset HF public et assumé. Double objectif : partage sur un outil ouvert + transparence sur les jeux de données qu'on emploie pour nos évaluations.

En face :

  • Les réunions internes (voix + verbatim = données personnelles, RGPD) ne vont jamais sur HF, même en privé (HF = société US, CLOUD Act).
  • Pour l'instant on n'utilise rien d'autre que HF : pas de S3 souverain ni de DVC. Les réunions internes restent locales (gitignorées), non versionnées en remote.

Conséquence : le garde-fou actuel bloque le cas d'usage visé

_ensure_private_repo refuse les repos publics et crée en private=True :

if not is_private:
    raise HuggingFaceError(
        f"Refusing to push to public dataset repo: {self.repo_id}. "
        "Benchmark corpora must stay private."
    )

Or c'est exactement le dataset public qu'on veut publier. La PR est conçue pour un scénario souverain qu'on abandonne (pour le moment). Donc ce n'est pas un petit fix, c'est un changement de design :

  1. Autoriser (voire viser par défaut) un repo public pour ce corpus. Idéalement un --public/--private explicite, défaut public pour ce dataset.
  2. Déplacer la sécurité côté contenu, pas côté repo. Comme il n'y a plus de séparation privé/public au niveau du repo, le vrai risque devient l'upload accidentel des réunions internes. push utilise aujourd'hui glob("*") sur tout data/audio + data/ground_truth : il faut le restreindre aux samples officiels explicitement listés (allowlist, sous-dossier dédié, ou manifeste public). C'est le point dur à câbler proprement.
  3. Mettre à jour la doc : README et .env.example disent « refuses to push to a public repo » et private=True, ce qui devient faux.

Autres remarques (code-level, indépendantes de la décision)

  • Bug d'aide CLI : --repo affiche defaults to ${HF_DEFAULT_DATASET_REPO}, interpolé sur la valeur, donc rendu defaults to $eval-transcript-corpus (ressemble à tort à une variable d'env). Mettre le littéral ou $HF_DATASET_REPO. Le --token -> $HF_TOKEN est correct, lui.
  • push est add-only, jamais delete : supprimer un sample en local puis push laisse le fichier obsolète sur le remote, alors que le README dit « Push the local corpus » (laisse croire à une synchro complète). À documenter, ou ajouter un CommitOperationDelete optionnel.
  • Ambiguïté multi-extension : _write_one prend candidates[0] sans tri si un sample a .wav et .mp3 (ordre de list_repo_files non garanti, donc non déterministe). Trier ou lever une erreur.
  • Sample partiel silencieux : audio sans ground truth (ou l'inverse) donne un PulledSample avec None, sans avertissement. Pour un corpus de bench, un sample sans référence est inexploitable : un warning serait utile.
  • --token en argument CLI : fuite possible via historique shell / ps / logs CI. Privilégier l'env ; un mot dans le README ne nuirait pas.
  • Incohérence de constantes : os.getenv("HF_DATASET_REPO", ...) utilise un littéral alors que DEFAULT_ORG_ENV / DEFAULT_API_KEY_ENV sont des constantes. Ajouter un DEFAULT_DATASET_REPO_ENV par symétrie.
  • uv.lock : la PR retire des wheels greenlet (s390x, riscv64). Probablement un refresh collatéral, mais c'est du bruit dans le diff : à confirmer que c'est voulu.

Points forts

  • Design testable via Protocol (HfApiLike) + injection api=, tests sans dépendance dure à huggingface_hub. Propre.
  • Garde-fous bien couverts (refus public, token requis, commit unique atomique, pull idempotent, filtrage des fichiers hors audio/ et ground_truth/).
  • Très bonne couverture de tests (constructeur, logique repo_id/org, list, pull streaming, push create+commit, helper download).
  • Lazy imports de huggingface_hub, erreurs typées, intégration CLI cohérente avec les autres providers.

En résumé : la mécanique est bonne, il faut surtout retourner la logique privé/public (public assumé pour les officiels) et garantir que les réunions internes ne puissent pas partir (allowlist côté contenu). Heureux d'en discuter de vive voix si tu veux.

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.

feat: add Hugging Face dataset pull/push provider

2 participants