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

Update sqlmodel #934

Closed
wants to merge 13 commits into from
Closed

Update sqlmodel #934

wants to merge 13 commits into from

Conversation

tcompa
Copy link
Collaborator

@tcompa tcompa commented Oct 27, 2023

Checklist before merging

  • I added an appropriate entry to CHANGELOG.md

@tcompa tcompa marked this pull request as draft October 27, 2023 12:14
@github-actions
Copy link

github-actions bot commented Oct 27, 2023

Coverage report

The coverage rate went from 93.18% to 93.29% ⬆️
The branch rate is 84%.

None of the new lines are part of the tested code. Therefore, there is no coverage data about them.

@tcompa
Copy link
Collaborator Author

tcompa commented Oct 27, 2023

First learning: the breaking (for our CI) change was in sqlmodel/main.py:

# sqlmodel 0.0.8
          if issubclass(field.type_, str):
              if field.field_info.max_length:
                  return AutoString(length=field.field_info.max_length)
              return AutoString
          if issubclass(field.type_, Enum):
              return sa_Enum(field.type_)

# sqlmodel 0.0.9
          if issubclass(field.type_, Enum):
              return sa_Enum(field.type_)
          if issubclass(field.type_, str):
              if field.field_info.max_length:
                  return AutoString(length=field.field_info.max_length)
              return AutoString

We have some objects like

fractal_server/app/models/job.py:class JobStatusType(str, Enum):
fractal_server/app/schemas/workflow.py:class WorkflowTaskStatusType(str, Enum):

which are both Enum and str subclasses. And therefore the order of the two if is relevant.

Refs:


Why is this relevant for postgres?
See red dragon here: https://docs.sqlalchemy.org/en/14/dialects/postgresql.html#prepared-statement-cache:

The asyncpg database driver necessarily uses caches for PostgreSQL type OIDs, which become stale when custom PostgreSQL datatypes such as ENUM objects are changed via DDL operations. Additionally, prepared statements themselves which are optionally cached by SQLAlchemy’s driver as described above may also become “stale” when DDL has been emitted to the PostgreSQL database which modifies the tables or other objects involved in a particular prepared statement.

The SQLAlchemy asyncpg dialect will invalidate these caches within its local process when statements that represent DDL are emitted on a local connection, but this is only controllable within a single Python process / database engine. If DDL changes are made from other database engines and/or processes, a running application may encounter asyncpg exceptions InvalidCachedStatementError and/or InternalServerError("cache lookup failed for type ") if it refers to pooled database connections which operated upon the previous structures. The SQLAlchemy asyncpg dialect will recover from these error cases when the driver raises these exceptions by clearing its internal caches as well as those of the asyncpg driver in response to them, but cannot prevent them from being raised in the first place if the cached prepared statement or asyncpg type caches have gone stale, nor can it retry the statement as the PostgreSQL transaction is invalidated when these errors occur.

@tcompa tcompa changed the title Debug sqlmodel 0.0.9 with postgres db Supdate sqlmodel Oct 30, 2023
@tcompa tcompa changed the title Supdate sqlmodel Update sqlmodel Oct 30, 2023
@tcompa
Copy link
Collaborator Author

tcompa commented Nov 10, 2023

Closing in favor of #949

@tcompa tcompa closed this Nov 10, 2023
@tcompa tcompa deleted the explore-sqlmodel-0.0.10 branch November 10, 2023 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant