Skip to content

Conversation

blanch0t
Copy link
Contributor

Reprise de la PR #376, mise à jour sur main (29/08/25)

revision: str = "788a3b052a11"
down_revision: Union[str, None] = "c1e54102255e"

revision: str = '479aeeae940b'

Check notice

Code scanning / CodeQL

Unused global variable Note

The global variable 'revision' is not used.
down_revision: Union[str, None] = "c1e54102255e"

revision: str = '479aeeae940b'
down_revision: Union[str, None] = '788a3b052a11'

Check notice

Code scanning / CodeQL

Unused global variable Note

The global variable 'down_revision' is not used.
Copy link
Contributor

@sladinji sladinji left a comment

Choose a reason for hiding this comment

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

Il manque un RabbitMQ dans les compose.yml

raise HTTPException(status_code=404, detail=f"Model '{model}' not found")

model = models[0]
model = await global_context.model_registry.list(model=model)[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Pourquoi avoir supprimé la gestion d'erreur si le model n'est pas trouvé ? On va se retrouver avec un IndexError alors qu'on avait une erreur explicite initialement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

je pense pas avoir fait exprès, j'ai remis

@@ -1,6 +1,6 @@
import json
from types import SimpleNamespace
from unittest.mock import AsyncMock
from unittest.mock import AsyncMock, MagicMock

Check notice

Code scanning / CodeQL

Unused import Note test

Import of 'MagicMock' is not used.
Copy link
Contributor

@sladinji sladinji left a comment

Choose a reason for hiding this comment

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

J'ai testé le code localement, et voici mes remarques :

  • Problème : les exceptions côté consumer (ex. WrongModelTypeException) se perdent dans le chemin RabbitMQ et apparaissent comme des timeouts côté appelant.
  • Correctif immédiat : dans _dispatch, catcher les erreurs, renvoyer l'exception vers le WorkingContext (p.ex. ctx.send_result(e)) et ack le message pour éviter qu'il soit re-queued :
except Exception as e:
    ctx.send_result(e)
    await message.ack()
  • Architecture : éviter les if configuration.dependencies.rabbitmq au cœur de ModelRegistry. Préférer une interface/abstraction (ex. ModelRegistryBase) et deux implémentations claires : ModelRegistrySync (sans queue) et ModelRegistryQueue (RabbitMQ). Cela rend le code plus testable et lisible.
  • Tests : ajouter des tests unitaires et d’intégration pour les deux modes (sync/queue). Plusieurs tests actuels échouent quand RabbitMQ est activé — couvrir explicitement la propagation d’erreur et le nettoyage des contexts.
  • Proposition long-terme : envisager Celery (https://docs.celeryq.dev/) au lieu d’un wiring RabbitMQ fait maison. Celery supporte plusieurs brokers (Redis ou RabbitMQ), backend de résultats, retries, et simplifie le choix du broker (on pourrait utiliser Redis en dev/CI et éviter d’ajouter une image RabbitMQ).
  • Ajouter des tests unitaires/intégrations (ça sera également plus simple à faire avec Celery)

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