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

Migrate CosmosDB code to Entity Framework #242

Open
dluc opened this issue Aug 23, 2023 · 4 comments
Open

Migrate CosmosDB code to Entity Framework #242

dluc opened this issue Aug 23, 2023 · 4 comments
Labels
enhancement New feature or request sk team issue

Comments

@dluc
Copy link
Collaborator

dluc commented Aug 23, 2023

I believe we could reduce the cost and complexity by using Entity Framework. That should allow to remove some code, not having to worry about security fixes and so on, plus EF is very mature and well known to most C#/DB devs, so we won't ask to learn a new/bespoke approach.

Some resources:

@dluc dluc changed the title Migrate to Entity Framework Migrate CosmosDB code to Entity Framework Aug 23, 2023
@dluc dluc added the enhancement New feature or request label Aug 23, 2023
@Rainson12
Copy link
Contributor

Rainson12 commented Sep 29, 2023

you might want to check Rainson12@7207c3e
I added EFcore support and use mariadb as storage. There might be 2 issues which i suboptimally fixed.

  1. the chatmessages has a property of type Dictionary
    public IDictionary<string, int>? TokenUsage { get; set; }
    which can not be mapped to an entity as there is no primary key and so entityframework doesnt like it hence I ignored the property for now. Also I added an empty constructor.
  2. The setup of the DB could be optimized. Due to the way on how the storageContexts are set, there will be 4 instances created (see Rainson12@7207c3e#diff-373907b269e2e6425302f31de796a82cde423beb713f62a7a73ed6ffefa08eacR204). Each instance only holds a dbset of the required entity, this allows for adding new entities without having to adjust the dbcontext however it has the downside of not being able to setup the database using EnsureCreated() as only the first table would be created, for every subsequent instantiation it wouldnt work, as the db is already created however the tables are not. I therefore check for the existance of the table and if it doesnt exist, create it using rawSQL generated by entityframework. see Rainson12@7207c3e#diff-4a03a8c447b6c719deb9e8de8d298990e7a6d3f3f67d57935fb8f83b8996ae81R32

Edit:
I separated the MariaDb Repository and DBContext because i think the db connection was deadlocking:
Rainson12@a2b53d0

@Ryangr0
Copy link

Ryangr0 commented Oct 13, 2023

As discussed in Chat Copilot #478 Can we come together on this? I'd love to see your PR @Rainson12 . I basically did the following for mysql

case ChatStoreOptions.ChatStoreType.MySql:
                {
                    if (chatStoreConfig.MySql == null)
                    {
                        throw new InvalidOperationException("ChatStore:MySql is required when ChatStore:Type is 'MySql'");
                    }

                    services.AddDbContext<MySqlDbContext>(
                        options =>
                            options.UseMySql(
                                chatStoreConfig.MySql.ConnectionString,
                                new MariaDbServerVersion(new Version(11, 1, 1))
                            // ,mySqlOptions => mySqlOptions.CharSetBehavior(CharSetBehavior.NeverAppend)
                            ),
                        ServiceLifetime.Transient
                    );

                    chatSessionStorageContext = new MySqlStorageContext<ChatSession>(services.BuildServiceProvider().GetRequiredService<MySqlDbContext>());
                    chatMessageStorageContext = new MySqlStorageContext<ChatMessage>(services.BuildServiceProvider().GetRequiredService<MySqlDbContext>());
                    chatMemorySourceStorageContext = new MySqlStorageContext<MemorySource>(services.BuildServiceProvider().GetRequiredService<MySqlDbContext>());
                    chatParticipantStorageContext = new MySqlStorageContext<ChatParticipant>(services.BuildServiceProvider().GetRequiredService<MySqlDbContext>());

                    services.AddScoped<IStorageContext<ChatSession>, MySqlStorageContext<ChatSession>>();
                    services.AddScoped<IStorageContext<ChatMessage>, MySqlStorageContext<ChatMessage>>();
                    services.AddScoped<IStorageContext<MemorySource>, MySqlStorageContext<MemorySource>>();
                    services.AddScoped<IStorageContext<ChatParticipant>, MySqlStorageContext<ChatParticipant>>();
                    break;
                }

and then

services.AddScoped<ChatSessionRepository>(sp => new ChatSessionRepository(chatSessionStorageContext));
        services.AddScoped<ChatMessageRepository>(sp => new ChatMessageRepository(chatMessageStorageContext));
        services.AddScoped<ChatMemorySourceRepository>(sp => new ChatMemorySourceRepository(chatMemorySourceStorageContext));
        services.AddScoped<ChatParticipantRepository>(sp => new ChatParticipantRepository(chatParticipantStorageContext));

But I'm really just winging it because I have no idea how concurrency issues play out in C# and EF because I've never used it before last week. There is probably a better solution.

@Rainson12
Copy link
Contributor

Hey @evchaki, why did this get closed? I couldn't find any solution and I think this conversation here is valuable for others as it seems like PRs didnt get merged but the user requirement is still a pressing one.

@evchaki evchaki reopened this Mar 20, 2024
@evchaki
Copy link
Contributor

evchaki commented Mar 20, 2024

@Rainson12 you are right, I closed this one by accident. Just reopened it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request sk team issue
Projects
None yet
Development

No branches or pull requests

5 participants