Skip to content

BE09: Implement AI module structure__Vedant#205

Merged
TienNguyen3711 merged 1 commit intomasterfrom
BE09-AI-Module-Structure
Apr 2, 2026
Merged

BE09: Implement AI module structure__Vedant#205
TienNguyen3711 merged 1 commit intomasterfrom
BE09-AI-Module-Structure

Conversation

@Vedant1515
Copy link
Copy Markdown
Collaborator

  1. Defined Structure - Organized AI code under ai with clear separation:

interfaces/ - Service contracts (ChatbotAI, MedicalPredictionAI, ImageClassificationAI, etc.)
clients/ - Implementations (External server, Python scripts, Groq/Chroma templates)
adapters/ - Main AIAdapter that controllers use
mocks/ - Mock implementations for testing

  1. Created AIAdapter - Single unified interface for all AI operations:

Controllers use: await aiAdapter.generateChatResponse()
No direct external calls anymore
Configuration-driven (mock vs. real services)

  1. Ready for Future - Groq LLM + Chroma RAG templates ready for Sprint 2

Controllers need ZERO changes when switching implementations

  1. No Breaking Changes - Completely non-intrusive:

AI team can continue current work uninterrupted
Backend can adopt gradually or immediately

5.Comprehensive Documentation - README, migration guide.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The constructor assigns this.isHealthy = true, but the class also defines an async isHealthy() method. That instance property will shadow the prototype method, so calls like client.isHealthy() in the adapter will fail at runtime. Please rename the state field so subclasses can still implement isHealthy() correctly.

Comment thread ai/adapters/AIAdapter.js
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The adapter exposes aiServerUrl and timeout as configurable values, but those settings are not actually passed into the ExternalAIServerClient instances. Right now new ExternalChatbotClient(null, this.config) and new ExternalMedicalPredictionClient(null, this.config) still create their own default server client, so the adapter config does not control the external base URL/timeout as documented.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This client passes timeout into fetchOptions, but native fetch does not use a timeout option. If timeouts are part of the contract for this module, this should be implemented with AbortController; otherwise requests may hang longer than the adapter/config suggests.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This catch block throws a plain object instead of an Error instance. That makes downstream error handling less reliable, because code expecting error.message, stack, or instanceof Error semantics will not behave consistently. Please throw an Error (or a dedicated custom error type) and attach metadata separately.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

executePythonScript expects timeoutMs, but this client is currently passing timeout. That means the configured timeout path from the adapter/options is not being applied as intended. Please rename this to timeoutMs so the Python execution timeout is actually honored. Also, this client has the same issue as the external server client: it throws a plain object instead of an Error. Converting failures into a real Error type would make the shared adapter/errorResponse flow much more predictable and keep stack/context handling cleaner.

Comment thread ai/SUMMARY.md
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The documentation currently describes this module as “production-ready” and “ready for immediate use,” but the branch still contains placeholder clients (Groq/Chroma), a missing referenced file (SPRINT_ROADMAP.md), and a few runtime issues in the core adapter/client path. I’d recommend softening this wording so the docs match the current implementation state.

@TienNguyen3711 TienNguyen3711 merged commit b61acc2 into master Apr 2, 2026
8 checks passed
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