Upload route using Django REST Framework#41
Upload route using Django REST Framework#41BSchelle wants to merge 24 commits intodataforgoodfr:dash-appfrom
Conversation
- localstack: latest → 3.3 (latest instable, bug Python 3.13/EKS) - ports: 4566 → 4566:4566 (exposition sur le host) - indentation: localstack placé sous services: (était au niveau racine) - ajout LOCALSTACK_ACKNOWLEDGE_ACCOUNT_REQUIREMENT=1 (sursis licence 2026) - ajout volume /var/run/docker.sock (requis par LocalStack)
…ad.y for clarity, updated dependencies requirements
| - ./docker/localstack/init-ready.d:/etc/localstack/init/ready.d:ro | ||
| image: localstack/localstack:3.3 #latest conflicted w/ front | ||
| ports: | ||
| - "4566:4566" #required listening port for |
There was a problem hiding this comment.
il ne faut pas utiliser le port de host : https://outline.services.dataforgood.fr/s/25739714-363d-4f17-8dba-632eb398b633#h-ii-lancer-une-application
Tu peux changer vers la valeur précédente :
ports:
- 4566
| **Lancer le containeur Docker** | ||
| ```bash | ||
| docker compose up -d | ||
| ``` |
There was a problem hiding this comment.
Cette partie est déjà écrite sous le point 3. Il y a une raison de refaire le lancement ?
| ce port. | ||
| Le bucket configuré est créé automatiquement au démarrage de LocalStack. | ||
|
|
||
| **Installer les dépendances et appliquer les migrations** |
There was a problem hiding this comment.
La section 4 décrit déjà l'installation de dépendances et la migration. C'est mieux de fusionne cette partie avec la section 4 pour éviter de confusion. Qu'est-ce que tu en penses ?
| app_name = "ingestion" | ||
| urlpatterns = [ | ||
| path("ingest/", views.ingest, name="ingest"), | ||
| path("api/upload/", upload.upload_pdf, name="api-upload"), |
There was a problem hiding this comment.
Le préfixe api/ sur la route upload crée une incohérence avec la route ingest/ dans la même app. Si ces deux endpoints sont des endpoints API, soit on applique le préfixe de manière cohérente aux deux, soit on le remonte au niveau du routeur parent.
Je pense qu'il serait préférable d'ajouter le préfixe dans le parent (eu_fact_force/app/urls.py) pour s'assurer que toutes les routes de cette sous-app sont cohéremment sous api/. Mais on peut faire une PR séparée pour l'implémenter.
| emails, | ||
| ): | ||
| def finalize_and_send(n_clicks, pdf_base64, filename, doi, abstract, journal, date, link, category, study_type, title, names, surnames, emails): | ||
| # Dash vérifie maintenant que pdf_base64 reçoit bien 'contents' et filename reçoit 'filename' |
There was a problem hiding this comment.
Question : j'ai vu dans le codebase que les commentaires sont écrits en anglais. Je n'ai pas trouvé des informations concernant le langue de codebase sur le wiki, du coup ma question est si on reste en anglais (et du coup traduire les commentaires et les messages d'api en anglais) ou on change vers français ?
Moi, je veux juste éviter un codebase franglais ^^
| finally: | ||
| tmp_path.unlink(missing_ok=True) # clean temp file | ||
|
|
||
| return Response({ |
There was a problem hiding this comment.
Bug potentiel: variables chunks et source_file non initialisées.
Si une exception est levée après l'appel à save_to_s3_and_postgres mais avant la fin de save_chunks (ou add_embeddings), le bloc except est bien déclenché et retourne une réponse d'erreur — jusque-là c'est correct.
Mais si jamais le flux d'exécution atteignait le return Response(...) final dans un cas limite ou une future refacto, chunks et source_file seraient des variables non définies, ce qui provoquerait un NameError en runtime.
Correction recommandée : initialiser les deux variables avant le bloc try :
source_file = None
chunks = []Ça coûte deux lignes et ça évite une bombe à retardement.
| content_type, content_string = pdf_base64.split(',') | ||
| pdf_bytes = base64.b64decode(content_string) | ||
|
|
||
| url = os.getenv("DJANGO_URL", "http://localhost:8000") + "/ingestion/api/upload/" |
There was a problem hiding this comment.
os.getenv('DJANGO_URL', 'http://localhost:8000') tombe silencieusement sur localhost si la variable d'environnement est absente en production. L'erreur sera découverte à l'exécution et non au démarrage de l'app.
Suggestion :
DJANGO_URL = os.environ["DJANGO_URL"] # KeyError si absent
# ou
DJANGO_URL = os.getenv("DJANGO_URL")
if not DJANGO_URL:
raise RuntimeError("DJANGO_URL env var is required")2 participants
I've updated Dash app to be able to upload PDF+metadatas to conteneurized local S3 and trigger parsing+embedding script to pgvector DB.
Wrote an appendix to readme to describe how to setup Dash webapp and create local bucket if not available.
Modified docker-compose localstack port to be listening from outside the container and volumes.
Modified urls.py to accept uploading PDFs
Modified pyproject.toml with up to date dependencies required for front to back pipeline.