apssouza22 / chatflow

Leveraging LLM to build Conversational UIs
BSD 2-Clause "Simplified" License
120 stars 26 forks source link

Apps in an SQL DB #56

Closed vporton closed 1 year ago

vporton commented 1 year ago

Intended to fix issue #7.

This is a work in progress. I created this PR to track its dependency:

Depends on PR #55.

I just write [WIP] in the title instead of creating a draft pull request, because I had some awkward experience with draft pull request, something about having a hardship to convert it to a normal PR.

vporton commented 1 year ago

I've implemented it but with two BUTS:

  1. not at all tested
  2. it does not use PK to query objects, as it should in clean code

I am not sure whether to test it now or first solve "2".

vporton commented 1 year ago

@apssouza22 I have a trouble finding Python code that loads source_data.json to the DB (that was a file DB in old code).

I noticed the file prepare_data.py but don't see in this file storing data in the file DB.

Please, help to find this code or understand why it is missing.

vporton commented 1 year ago

Now create app and list apps work.

I propose you @apssouza22 to merge this PR to main, because so we will find the rest bugs, if any, during working with main branch.

apssouza22 commented 1 year ago

@vporton it is hard to test all these changes. Could you remove Alckemy from this MR? Make it simple. Addressing only one thing(store apps in the database)

On the opensource world we need to make PRs very easy small and easy to follow because mantainers don't have time to dig into big changes

vporton commented 1 year ago

it is hard to test all these changes. Could you remove Alckemy from this MR? Make it simple.

It's not very easy to remove Alchemy from this set of changes because of subtle differences, but well I will do.

vporton commented 1 year ago

I removed SQLAlchemy code.

Please, accept this PR. It may contain errors, but in the present time the main method of software assurance is testing, not mathematical proof that it's correct. So, we will have enough time to test it, when it will be in the main branch, because our project is still unfinished.

apssouza22 commented 1 year ago

@vporton the app works fine today. We can not merge anything that breaks it.

I have pulled your branch the app is not even starting. Please do some basic taste before open the PR.

chatflow-postgres-1  | 2023-10-22 19:53:14.690 UTC [60] DETAIL:  Failing row contains (null, admin@gmail.com, 123, Alex).

Before that I had to fix the create table script

CREATE TABLE users (
    id integer primary key NOT NULL,
vporton commented 1 year ago

@apssouza22 Please, create branch develop to integrate there changes that were not yet tested (instead of handing multiple non-merged pull requests).

Fixed table users.

This error can be safely ignored. I tried to handle it with except: but in some reason it is not caught:

        try:
            self.add_user(User(name="Alex", password="123", email="admin@gmail.com"))
            self.dao.db.conn.commit()
        except IntegrityError as e:
            print("IGNORED EXCEPTION", e)  # TODO: Remove this line.
            self.dao.db.conn.rollback()
vporton commented 1 year ago

@apssouza22 Oh, sorry, there really was a bug with DETAIL: Failing row contains. It is because of a wrong SQL code, and I did not notice it, because I already executed correct code in the past. Now it is fixed.

vporton commented 1 year ago

Superseded by PR #62.