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

the modules with controllers were modified to be dynamic #85

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion apps/api/src/app/chat/chat.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,13 @@ export const assistantParams: AssistantCreateParams = {
export const assistantConfig: AssistantConfigParams = {
id: process.env['ASSISTANT_ID'] || '',
params: assistantParams,
assistantPrefix: 'assistant-prefix',
Copy link
Member

@sebastianmusial sebastianmusial Dec 17, 2024

Choose a reason for hiding this comment

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

Can you put default assistant-prefix as an optional value - empty string?

Copy link
Member

Choose a reason for hiding this comment

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

image Please check my screenshot, It looks like we have multiple instances instead of one with the prefix 🤔

In my opinion we should have only once instance and decide if we would like have prefix or not.

filesDir: './apps/api/src/app/knowledge',
toolResources: {
fileSearch: {
fileNames: ['33-things-to-ask-your-digital-product-development-partner.md'],
fileNames: [
'33-things-to-ask-your-digital-product-development-partner.md',
],
},
codeInterpreter: {
fileNames: [],
Expand Down
2 changes: 1 addition & 1 deletion apps/api/src/app/chat/chat.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { AgentsModule } from './agents/agents.module';
import { ChatSockets } from './chat.sockets';

@Module({
imports: [AgentsModule, AssistantModule.forRoot(assistantConfig)],
imports: [AgentsModule, AssistantModule.register(assistantConfig)],
Copy link
Member

Choose a reason for hiding this comment

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

Could you update the readme file:

  • forRoot -> register
  • new property in the configuration

providers: [ChatSockets],
})
export class ChatModule {}
20 changes: 18 additions & 2 deletions libs/openai-assistant/src/lib/ai/ai.module.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,26 @@
import { Module } from '@nestjs/common';
import { DynamicModule, Module } from '@nestjs/common';
import { AiService } from './ai.service';
import { AiController } from './ai.controller';
import { createControllerWithPrefix } from '../../utils/controllers';

@Module({
providers: [AiService],
controllers: [AiController],
Copy link
Member

Choose a reason for hiding this comment

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

In that case we have multiple controllers, please remove it.

exports: [AiService],
})
export class AiModule {}
export class AiModule {
static register(prefix: string): DynamicModule {
return {
module: AiModule,
providers: [
AiService,
{
provide: 'PREFIX',
useValue: prefix,
},
],
controllers: [createControllerWithPrefix(AiController, prefix)],
exports: [AiService],
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import { createReadStream } from 'fs';
import { AiService } from '../ai';
import { ConfigService } from '../config';
import { AssistantCreateParams, VectorStore } from 'openai/resources/beta';
import ToolResources = AssistantCreateParams.ToolResources;
import { AssistantUpdateParams } from 'openai/resources/beta/assistants';
import {
AssistantCodeInterpreter,
Expand Down Expand Up @@ -41,7 +40,7 @@ export class AssistantFilesService {
async getCodeInterpreterResources(
data: AssistantCodeInterpreter,
fileDir = '',
): Promise<ToolResources.CodeInterpreter> {
): Promise<AssistantCreateParams.ToolResources.CodeInterpreter> {
const files = await this.getFiles(data?.fileNames, fileDir);

return { file_ids: files.map(({ id }) => id) };
Expand All @@ -50,7 +49,7 @@ export class AssistantFilesService {
async getFileSearchResources(
data: AssistantFileSearch,
fileDir = '',
): Promise<ToolResources.FileSearch> {
): Promise<AssistantCreateParams.ToolResources.FileSearch> {
if (!data) {
return { vector_store_ids: [] };
}
Expand Down
1 change: 1 addition & 0 deletions libs/openai-assistant/src/lib/assistant/assistant.model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ export interface AssistantConfigParams {
params: AssistantCreateParams;
options?: RequestOptions;
filesDir?: string;
assistantPrefix?: string;
toolResources?: AssistantToolResources | null;
}

Expand Down
29 changes: 19 additions & 10 deletions libs/openai-assistant/src/lib/assistant/assistant.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,20 +26,14 @@ const sharedServices = [
AssistantMemoryService,
];

const sharedModules = [
const sharedModulesWithoutControllers = [AgentModule, RunModule];
const sharedModulesWithControllers = [
AiModule,
AgentModule,
RunModule,
FilesModule,
ThreadsModule,
FilesModule,
ChatModule,
];

@Module({
imports: [ConfigModule, HttpModule, ...sharedModules],
providers: [...sharedServices],
exports: [...sharedServices, ...sharedModules],
})
export class AssistantModule implements OnModuleInit {
constructor(
private readonly assistantService: AssistantService,
Expand All @@ -56,14 +50,29 @@ export class AssistantModule implements OnModuleInit {
await this.assistantService.init();
}

static forRoot(config: AssistantConfigParams): DynamicModule {
static register(config: AssistantConfigParams): DynamicModule {
return {
module: AssistantModule,
imports: [
ConfigModule,
HttpModule,
...sharedModulesWithoutControllers,
...sharedModulesWithControllers.map(module =>
module.register(config?.assistantPrefix || ''),
),
],
providers: [
{
provide: 'config',
useValue: config,
},
...sharedServices,
],

exports: [
...sharedServices,
...sharedModulesWithoutControllers,
...sharedModulesWithControllers,
],
};
}
Expand Down
2 changes: 2 additions & 0 deletions libs/openai-assistant/src/lib/chat/chat.controller.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import { Body, Controller, Post } from '@nestjs/common';
import { Type } from '@nestjs/common/interfaces';

import { ApiBody, ApiResponse, ApiTags } from '@nestjs/swagger';
import { ChatCallDto, ChatCallResponseDto } from './chat.model';
import { ChatService } from './chat.service';
Expand Down
27 changes: 19 additions & 8 deletions libs/openai-assistant/src/lib/chat/chat.module.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,28 @@
import { Module } from '@nestjs/common';
import { DynamicModule } from '@nestjs/common';
import { AiModule } from '../ai';
import { RunModule } from '../run';
import { ChatHelpers } from './chat.helpers';
import { ChatService } from './chat.service';
import { SocketModule } from '@nestjs/websockets/socket-module';
import { ChatController } from './chat.controller';
import { createControllerWithPrefix } from '../../utils/controllers';

export const sharedServices = [ChatHelpers, ChatService];

@Module({
imports: [SocketModule, AiModule, RunModule],
providers: [...sharedServices],
controllers: [ChatController],
exports: [...sharedServices],
})
export class ChatModule {}
export class ChatModule {
static register(prefix: string): DynamicModule {
return {
module: ChatModule,
imports: [SocketModule, AiModule, RunModule],
providers: [
...sharedServices,
{
provide: 'PREFIX',
useValue: prefix,
},
],
controllers: [createControllerWithPrefix(ChatController, prefix)],
exports: [...sharedServices],
};
}
}
20 changes: 18 additions & 2 deletions libs/openai-assistant/src/lib/files/files.module.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,28 @@
import { Module } from '@nestjs/common';
import { DynamicModule, Module } from '@nestjs/common';
import { FilesService } from './files.service';
import { FilesController } from './files.controller';
import { AiModule } from '../ai';
import { createControllerWithPrefix } from '../../utils/controllers';

@Module({
imports: [AiModule],
providers: [FilesService],
controllers: [FilesController],
Copy link
Member

Choose a reason for hiding this comment

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

In that case we have multiple controllers, please remove it.

exports: [FilesService],
})
export class FilesModule {}
export class FilesModule {
static register(prefix: string): DynamicModule {
return {
module: FilesModule,
providers: [
AiModule,
{
provide: 'PREFIX',
useValue: prefix,
},
],
controllers: [createControllerWithPrefix(FilesController, prefix)],
exports: [FilesService],
};
}
}
20 changes: 18 additions & 2 deletions libs/openai-assistant/src/lib/threads/threads.module.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,28 @@
import { Module } from '@nestjs/common';
import { DynamicModule, Module } from '@nestjs/common';
import { ThreadsController } from './threads.controller';
import { ThreadsService } from './threads.service';
import { AiModule } from '../ai';
import { createControllerWithPrefix } from '../../utils/controllers';

@Module({
imports: [AiModule],
providers: [ThreadsService],
controllers: [ThreadsController],
Copy link
Member

Choose a reason for hiding this comment

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

In that case we have multiple controllers, please remove it.

exports: [ThreadsService],
})
export class ThreadsModule {}
export class ThreadsModule {
static register(prefix: string): DynamicModule {
return {
module: ThreadsModule,
providers: [
ThreadsService,
{
provide: 'PREFIX',
useValue: prefix,
},
],
controllers: [createControllerWithPrefix(ThreadsController, prefix)],
exports: [ThreadsService],
};
}
}
35 changes: 35 additions & 0 deletions libs/openai-assistant/src/utils/controller.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { Controller, Get } from '@nestjs/common';
import { createControllerWithPrefix } from './controllers';
import 'reflect-metadata';

@Controller('test')
class TestController {
@Get()
getTest(): string {
return 'Original Controller';
}
}

describe('createControllerWithPrefix', () => {
it('should create a new controller with the prefixed route', () => {
const PrefixedController = createControllerWithPrefix(
TestController,
'api',
);

const originalPath = Reflect.getMetadata('path', TestController);
const prefixedPath = Reflect.getMetadata('path', PrefixedController);

expect(originalPath).toBe('test');

expect(prefixedPath).toBe('api/test');
Copy link
Member

Choose a reason for hiding this comment

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

I can't image the case that we would like to keep both version - with and without prefix. IMO user should be able to decide and select only one option by providing prefix or not.

});

it('should throw an error if the controller does not have @Controller decorator', () => {
class InvalidController {}

expect(() => createControllerWithPrefix(InvalidController, 'api')).toThrow(
'The provided controller does not have a @Controller decorator.',
);
});
});
24 changes: 24 additions & 0 deletions libs/openai-assistant/src/utils/controllers.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { Controller } from '@nestjs/common';
import { Type } from '@nestjs/common/interfaces';
import 'reflect-metadata';

export function createControllerWithPrefix<T extends object>(
controller: Type<T>,
prefix: string,
): Type<T> {
const originalPath = Reflect.getMetadata('path', controller) as
| string
| undefined;

if (!originalPath) {
throw new Error(
'The provided controller does not have a @Controller decorator.',
);
}

const DynamicController = class extends controller {};

Controller(`${prefix}/${originalPath}`)(DynamicController);

return DynamicController;
}
Loading