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

Review enum-related behavior #935

Closed
tcompa opened this issue Oct 27, 2023 · 6 comments · Fixed by #974
Closed

Review enum-related behavior #935

tcompa opened this issue Oct 27, 2023 · 6 comments · Fixed by #974
Assignees
Labels
1.4.0 db life cycle Migrations and co dependencies Pull requests that update a dependency file High Priority Current Priorities & Blocking Issues

Comments

@tcompa
Copy link
Collaborator

tcompa commented Oct 27, 2023

As of #934 (comment), we may be fixing the CI with sqlmodel >0.0.8, but we still need to make sure that the following test works.

  • Add a migration that changes job.status type into an actual enum, as it would be autogenerated with sqlmodel 0.0.9
  • Run fractal-server with postgres (and gunicorn, if possible)
  • Look for unexpected behavior, especially by running jobs (because it's their status that could be broken)
  • In case of errors, try disabling asyncpg cache (https://docs.sqlalchemy.org/en/14/dialects/postgresql.html#prepared-statement-cache)
@tcompa tcompa added the dependencies Pull requests that update a dependency file label Oct 27, 2023
@tcompa
Copy link
Collaborator Author

tcompa commented Oct 27, 2023

Note: due to the bug in sqlmodel<=0.0.8, that was representing enums as autostrings, we have no enums in our migrations.
Therefore this specific error should be impossible until we update sqlmodel to >=0.0.9.

@tcompa
Copy link
Collaborator Author

tcompa commented Oct 27, 2023

Ref tiangolo/sqlmodel#164

@mfranzon mfranzon self-assigned this Nov 8, 2023
@mfranzon
Copy link
Collaborator

mfranzon commented Nov 8, 2023

As long as SQLITE does not support ENUM type, do we want to make an if branch to store enum in postgres and string in sqlite?

The other possibility is to remove Enum from JobStatusType model, and make it a string.

@tcompa
Copy link
Collaborator Author

tcompa commented Nov 8, 2023

Where would you place the if? In the migration scripts?

@mfranzon
Copy link
Collaborator

mfranzon commented Nov 8, 2023

Where would you place the if? In the migration scripts?

Yes, I am thinking the same but honestly I don't like too much this approach

@tcompa tcompa added the 1.4.0 label Nov 16, 2023
@tcompa
Copy link
Collaborator Author

tcompa commented Nov 16, 2023

We re-discussed this with @mfranzon, and had a look at known issues like:

  • We think it's not worth it to have actual enums in the db, for the moment. We'll keep them for all serializer (that is, Pydantic models), but not for the actual tables (that is, SQLModel models).
  • Let's add a Pydantic validator to the SQLModel objects, that checks e.g. that ApplyWorkflow.status complies with JobStatusType.

In this way we don't risk breaking changes with due to the recent sqlmodel update.

We should also keep it in mind when adding new columns in the future.

@tcompa tcompa added the High Priority Current Priorities & Blocking Issues label Nov 16, 2023
@tcompa tcompa linked a pull request Nov 16, 2023 that will close this issue
1 task
@tcompa tcompa added the db life cycle Migrations and co label Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.4.0 db life cycle Migrations and co dependencies Pull requests that update a dependency file High Priority Current Priorities & Blocking Issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants