ovh / celery-director

Simple and rapid framework to build workflows with Celery
https://ovh.github.io/celery-director/
BSD 3-Clause "New" or "Revised" License
534 stars 58 forks source link

feat: add description field to workflows #162

Closed henri42 closed 1 year ago

henri42 commented 2 years ago

In order to improve the monitoring experience on the UI I added a description field to the workflows table. This way it is possible to add specific information to each workflow instance, and distinguish between the different runs.

Of course, the description is optional. You can run a workflow with description in three ways :

1. Using CLI

director workflow run product.ORDER '{"user": 1234, "product": 1000}' --description "short description here"

2. Using API

curl --header "Content-Type: application/json" \
  --request POST \
  --data '{"project": "product", "name": "ORDER", \
    "payload": {"user": 1234, "product": 1000}, \
    "description": "short description here"}' \
  http://localhost:8000/api/workflows

3. Using UI, on the "Definition" page

desc-ui

After running at least one workflow with a description you will be able to see the description in the web UI

desc-wf-list desc-details


Two remarks:

Thanks for your reading

ncrocfer commented 2 years ago

Hi @henri42,

Thank you for this PR ! We'll check it this week before giving you our feedbacks ;)

henri42 commented 2 years ago

Hi ! Thanks for your review.

I tried your suggestion, it is ok for existing instances (at least upgraded to 2ac615d6850b_force_varchar_255.py) but not for new ones.

If I apply the revision 9d563aaa548b_add_workflows_description.py at the end of the history, it causes the following error when upgrading the revision 2ac615d6850b_force_varchar_255.py.

sqlalchemy.exc.OperationalError: (sqlite3.OperationalError) no such column: workflows.description
[SQL: INSERT INTO _alembic_tmp_workflows (id, created_at, updated_at, name, project, status, payload, periodic, description) SELECT workflows.id, workflows.created_at, workflows.updated_at, workflows.name, workflows.project, workflows.status, workflows.payload, workflows.periodic, workflows.description 
FROM workflows]

After investigating, it looks like this is caused by the following block in 2ac615d6850b_force_varchar_255.py:

with op.batch_alter_table("workflows", copy_from=tables["workflows"]) as batch_op:
        batch_op.alter_column(
            "name",
            existing_type=sa.VARCHAR(),
            type_=sa.String(length=255),
            existing_nullable=False,
        )
        batch_op.alter_column(
            "project",
            existing_type=sa.VARCHAR(),
            type_=sa.String(length=255),
            existing_nullable=False,
        )

Indeed, when printing the columns list during the 2ac615d6850b_force_varchar_255.py revision, the workflows.description column is already present before the creation of the table in the last revision 9d563aaa548b_add_workflows_description.py :

print(tables["workflows"].columns)
>>> ['workflows.id', 'workflows.created_at', 'workflows.updated_at', 'workflows.name', 'workflows.project', 'workflows.status', 'workflows.payload', 'workflows.periodic', 'workflows.description']

I think it is logic because it reads the table from the models in the code where the description column is present.

Removing the parameter copy_from=tables["workflows"] resolves the problem, but I saw that you added it on purpose to fix the offline mode problem with SQLite. (see #85)

How do you think we can manage that ? Currently, it does not seem to be possible to add new columns to the existing tables on the project.

Henri

ncrocfer commented 2 years ago

Hi @henri42 ,

Indeed you raised a bug in Celery Director :relaxed:

I created a new issue #164 , we'll talk with the team about a possible solution. But for me the problem only comes from sqlite engine, and especially the need to generate flat SQL files.

If using the online mode of Alembic fixes this problem, but brings another one (the impossibility to add new column) I think we'll simply switch to offline mode.

I will keep you updated about the internal discussion. As a Celery Director user what do you think about it?

henri42 commented 2 years ago

In our current setup we are using a PostgreSQL database so loosing the SQLite offline feature will not be a problem, and we have a little team with no database Administrator.

In terms of future evolution, I think that the ability to modify an existing table could make sense.

ncrocfer commented 1 year ago

Hi @henri42 ,

We finally fixed the #164 issue, sorry for the delay. Can you please rebase your branch on master ?

And can you also change the description word into comment? Because later we'll certainly add a description field in the YAML to describe the objective of each workflow.

ncrocfer commented 1 year ago

Is-it ok for you to rebase your PR and change description in comment @henri42 ? If you prefer I can also close this PR so you will be able to create another one :)

henri42 commented 1 year ago

Yes, no problem I will rebase this PR soon. Thank you for fixing the issue !

henri42 commented 1 year ago

Hi @ncrocfer, it is rebased and the name has been fixed.