Skip to content

refactor(robot-server): Avoid features that will be removed in SQLAlchemy 2.0 #16926

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

Merged
merged 70 commits into from
Nov 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
70 commits
Select commit Hold shift + click to select a range
282c99c
WIP intial schema and orchestrator changes
TamarZanzouri Nov 1, 2024
1bb8d7c
changed schema and imports to 8
TamarZanzouri Nov 1, 2024
77902c6
insert the errors to the DB
TamarZanzouri Nov 4, 2024
15db246
refactor into commands table
TamarZanzouri Nov 4, 2024
b88e722
select from commands
TamarZanzouri Nov 4, 2024
b6d33b6
linting and command error slice done
TamarZanzouri Nov 5, 2024
85eda17
added tests and fixed bugs
TamarZanzouri Nov 5, 2024
5bc0500
changed logic for current command errors
TamarZanzouri Nov 5, 2024
e423f37
removed failed_command_errors from state
TamarZanzouri Nov 5, 2024
e29d95f
refactor get command errors
TamarZanzouri Nov 5, 2024
c275c35
Merge branch 'edge' into EXEC-655-store-commands-error-list-in-db
TamarZanzouri Nov 6, 2024
a950cb8
migration and initialization fixes
TamarZanzouri Nov 6, 2024
55a809a
Update robot-server/robot_server/runs/run_store.py
TamarZanzouri Nov 6, 2024
1ccaba3
Merge branch 'edge' into EXEC-655-store-commands-error-list-in-db
TamarZanzouri Nov 6, 2024
9fa97ba
change get_run_errors to get count
TamarZanzouri Nov 6, 2024
f5fe016
Merge branch 'edge' into EXEC-655-store-commands-error-list-in-db
TamarZanzouri Nov 6, 2024
13c5b87
tables tests fixed
TamarZanzouri Nov 6, 2024
a88e95d
added test for historical run
TamarZanzouri Nov 6, 2024
6fea08a
Merge branch 'edge' into EXEC-655-store-commands-error-list-in-db
TamarZanzouri Nov 6, 2024
1bea28e
fixed persistance path
TamarZanzouri Nov 6, 2024
8f73bc1
fixed import from schema 7
TamarZanzouri Nov 7, 2024
c87dc65
rollback add_column change
TamarZanzouri Nov 7, 2024
5405ef3
append_failed_command_id when setting a failed command
TamarZanzouri Nov 7, 2024
f888364
append_failed_command_id when setting a failed command
TamarZanzouri Nov 7, 2024
e6aa375
moved test to command state
TamarZanzouri Nov 8, 2024
66856ac
changed index and lint
TamarZanzouri Nov 8, 2024
02e6806
WIP - command status
TamarZanzouri Nov 8, 2024
37a4aff
fixed failing tests
TamarZanzouri Nov 8, 2024
6c6257f
insert and get with command_status
TamarZanzouri Nov 8, 2024
56d12c4
translate to CommandStatusSQLEnum
TamarZanzouri Nov 8, 2024
c33745b
nips
TamarZanzouri Nov 12, 2024
635a5af
Update robot-server/tests/runs/test_run_store.py
TamarZanzouri Nov 12, 2024
a16636b
nits
TamarZanzouri Nov 12, 2024
a873369
batch edit
TamarZanzouri Nov 13, 2024
67b6d53
batch update without binding
TamarZanzouri Nov 13, 2024
4d00c99
test table with new index and migration bulk fix
TamarZanzouri Nov 13, 2024
38cd6ea
conditional update
TamarZanzouri Nov 13, 2024
1160174
map command status to sql command status
TamarZanzouri Nov 13, 2024
18ce80c
map command status to sql command status in migration
TamarZanzouri Nov 13, 2024
964fddc
map command status to sql command status in migration
TamarZanzouri Nov 13, 2024
5ad4d0d
add missing indexes
TamarZanzouri Nov 13, 2024
de02268
lint
TamarZanzouri Nov 13, 2024
085d4bb
change conversion into enum
TamarZanzouri Nov 13, 2024
378eec1
Merge branch 'edge' into EXEC-655-store-commands-error-list-in-db
TamarZanzouri Nov 13, 2024
ec64c12
fixing merge conflicts - WIP
TamarZanzouri Nov 14, 2024
b285c99
WIP - fix test_tables failing
TamarZanzouri Nov 14, 2024
3a29bfd
update test table
TamarZanzouri Nov 20, 2024
dca8269
Revert "update test table"
TamarZanzouri Nov 20, 2024
4d91d6f
Fix schema_8.py after merge.
SyntaxColoring Nov 14, 2024
fd78ad3
Fix up test_tables.py after merge.
SyntaxColoring Nov 14, 2024
b59601e
Remove attempts to fix prior migrations, for now.
SyntaxColoring Nov 20, 2024
6496d3c
Make command_status column nullable to match what the migration does.
SyntaxColoring Nov 20, 2024
11aa280
Remove enum constraint, for now.
SyntaxColoring Nov 20, 2024
5af324e
Merge branch 'edge' into EXEC-655-store-commands-error-list-in-db
SyntaxColoring Nov 20, 2024
cf88696
Treat SQLAlchemy deprecation warnings as errors.
SyntaxColoring Nov 20, 2024
afbe61e
Deduplicate add_column() and avoid deprecated features.
SyntaxColoring Nov 20, 2024
f6b61b7
Deduplicate index declaration and avoid executing raw string.
SyntaxColoring Nov 21, 2024
6669e61
Update robot-server/robot_server/persistence/_migrations/v7_to_v8.py
TamarZanzouri Nov 21, 2024
ce3fbb3
Update robot-server/robot_server/runs/run_store.py
TamarZanzouri Nov 21, 2024
e0aacbd
select subquery and minor fixes
TamarZanzouri Nov 21, 2024
aa720f8
We love to lint.
SyntaxColoring Nov 21, 2024
b10e8a2
fixed join logic and added logic to the tests
TamarZanzouri Nov 22, 2024
13fd3a8
fixed logic of cursor
TamarZanzouri Nov 22, 2024
55d190e
remove join
TamarZanzouri Nov 25, 2024
68cfdf7
Update robot-server/robot_server/runs/run_store.py
TamarZanzouri Nov 25, 2024
70733de
Merge branch 'edge' into EXEC-655-store-commands-error-list-in-db
TamarZanzouri Nov 25, 2024
86510ec
import from schema_7
TamarZanzouri Nov 25, 2024
73b6ac7
Todo comment for getting SQLAlchemy to compile the text for us.
SyntaxColoring Nov 25, 2024
36186b9
Merge commit '86510ec0383796e8ca9f205b77dbb689aeb3d4ae' into forwards…
SyntaxColoring Nov 25, 2024
5148daf
Merge branch 'edge' into forwards_compatible_sqlalchemy
SyntaxColoring Nov 25, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions robot-server/pytest.ini
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,8 @@ markers =
ot3_only: Test only functions using the OT3 hardware
addopts = --color=yes --strict-markers
asyncio_mode = auto

# Don't allow any new code that uses features removed in SQLAlchemy 2.0.
# We should remove this when we upgrade to SQLAlchemy 2.0.
filterwarnings =
error::sqlalchemy.exc.RemovedIn20Warning
45 changes: 43 additions & 2 deletions robot-server/robot_server/persistence/_migrations/_util.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
"""Shared helpers for migrations."""

import sqlalchemy

import shutil
import typing
from pathlib import Path

import sqlalchemy

from ..database import sqlite_rowid


Expand Down Expand Up @@ -54,3 +55,43 @@ def copytree_if_exists(src: Path, dst: Path) -> None:
shutil.copytree(src=src, dst=dst)
except FileNotFoundError:
pass


def add_column(
engine: sqlalchemy.engine.Engine,
table_name: str,
column: typing.Any,
) -> None:
"""Add a column to an existing SQL table, with an `ALTER TABLE` statement.

Params:
engine: A SQLAlchemy engine to connect to the database.
table_name: The SQL name of the parent table.
column: The SQLAlchemy column object.

Known limitations:

- This does not currently support indexes.
- This does not currently support constraints.
- The column will always be added as nullable. Adding non-nullable columns in
SQLite requires an elaborate and sensitive dance that we do not wish to attempt.
https://www.sqlite.org/lang_altertable.html#making_other_kinds_of_table_schema_changes

To avoid those limitations, instead of this function, consider this:

1. Start with an empty database, or drop or rename the current table.
2. Use SQLAlchemy's `metadata.create_all()` to create an empty table with the new
schema, including the new column.
3. Copy rows from the old table to the new one, populating the new column
however you please.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol, this is what Alembic is for.

But I wonder if there's a way to use Alembic as a fancy SQL generator for diffs without buying into the full Alembic ecosystem.

Copy link
Contributor Author

@SyntaxColoring SyntaxColoring Nov 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol, this is what Alembic is for.

That is what we thought, but I looked into it when working on this, and I was underwhelmed. Alembic does implement this dance, and that is appealing. But:

First, like you said, there's a full Alembic ecosystem that we'd have to contend with. In particular, we would probably want to run Alembic "inside" our existing migration system. (It can't completely replace our own migration system, because we need to account for migrating regular files, and Alembic can only help with the schema inside the .db file.) Embedding it like that...seems messy? Like, one hypothetical way for it to work would be if Alembic gave us a standalone util function like alembic.add_all_column_constraints(command_table.c.command_status), and we'd call that from within our existing system, but it doesn't seem like Alembic works like that.

Second, as far as I can tell, Alembic leaves us on our own for the data part of these migrations, e.g. populating the new non-nullable column. They recommend some general patterns in sqlalchemy/alembic#972 (reply in thread) and https://alembic.sqlalchemy.org/en/latest/cookbook.html#data-migrations-general-techniques. Those patterns strike me as their own dances that are only marginally better. Especially if we need to rearchitect to fit into the Alembic ecosystem just for the privilege of using them.

But I could definitely have big misconceptions about all of this. I've never actually used Alembic for real. If you want to make a sketch or proof of concept to show what it would look like, I'd definitely love something better than what we have now.

But I wonder if there's a way to use Alembic as a fancy SQL generator for diffs without buying into the full Alembic ecosystem.

Yeah. I'm not sure if this what you're getting at, but there is https://alembic.sqlalchemy.org/en/latest/autogenerate.html, and we might be able to combine that with https://alembic.sqlalchemy.org/en/latest/offline.html. So one option is to run Alembic once on our laptops to autogenerate the ALTER TABLE dance, and then manually integrate that with our data migrations. Is that what you have in mind?

"""
column_type = column.type.compile(engine.dialect)
with engine.begin() as transaction:
# todo(mm, 2024-11-25): This text seems like something that SQLAlchemy could generate for us
# (maybe: https://docs.sqlalchemy.org/en/20/core/metadata.html#sqlalchemy.schema.Column.compile),
# and that might help us account for indexes and constraints.
transaction.execute(
sqlalchemy.text(
f"ALTER TABLE {table_name} ADD COLUMN {column.key} {column_type}"
)
)
14 changes: 1 addition & 13 deletions robot-server/robot_server/persistence/_migrations/v3_to_v4.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,8 @@
from pathlib import Path
from contextlib import ExitStack
import shutil
from typing import Any

import sqlalchemy

from ._util import add_column
from ..database import sql_engine_ctx
from ..file_and_directory_names import DB_FILE
from ..tables import schema_4
Expand All @@ -35,16 +33,6 @@ def migrate(self, source_dir: Path, dest_dir: Path) -> None:
dest_engine = exit_stack.enter_context(sql_engine_ctx(dest_db_file))
schema_4.metadata.create_all(dest_engine)

def add_column(
engine: sqlalchemy.engine.Engine,
table_name: str,
column: Any,
) -> None:
column_type = column.type.compile(engine.dialect)
engine.execute(
f"ALTER TABLE {table_name} ADD COLUMN {column.key} {column_type}"
)

add_column(
dest_engine,
schema_4.analysis_table.name,
Expand Down
14 changes: 1 addition & 13 deletions robot-server/robot_server/persistence/_migrations/v4_to_v5.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,8 @@
from pathlib import Path
from contextlib import ExitStack
import shutil
from typing import Any

import sqlalchemy

from ._util import add_column
from ..database import sql_engine_ctx
from ..file_and_directory_names import DB_FILE
from ..tables import schema_5
Expand All @@ -35,16 +33,6 @@ def migrate(self, source_dir: Path, dest_dir: Path) -> None:
dest_engine = exit_stack.enter_context(sql_engine_ctx(dest_db_file))
schema_5.metadata.create_all(dest_engine)

def add_column(
engine: sqlalchemy.engine.Engine,
table_name: str,
column: Any,
) -> None:
column_type = column.type.compile(engine.dialect)
engine.execute(
f"ALTER TABLE {table_name} ADD COLUMN {column.key} {column_type}"
)

add_column(
dest_engine,
schema_5.protocol_table.name,
Expand Down
12 changes: 1 addition & 11 deletions robot-server/robot_server/persistence/_migrations/v6_to_v7.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@
from pathlib import Path
from contextlib import ExitStack
import shutil
from typing import Any

import sqlalchemy

from ._util import add_column
from ..database import sql_engine_ctx, sqlite_rowid
from ..tables import schema_7
from .._folder_migrator import Migration
Expand Down Expand Up @@ -44,16 +44,6 @@ def migrate(self, source_dir: Path, dest_dir: Path) -> None:

dest_transaction = exit_stack.enter_context(dest_engine.begin())

def add_column(
engine: sqlalchemy.engine.Engine,
table_name: str,
column: Any,
) -> None:
column_type = column.type.compile(engine.dialect)
engine.execute(
f"ALTER TABLE {table_name} ADD COLUMN {column.key} {column_type}"
)

add_column(
dest_engine,
schema_7.run_command_table.name,
Expand Down
19 changes: 6 additions & 13 deletions robot-server/robot_server/persistence/_migrations/v7_to_v8.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@
from pathlib import Path
from contextlib import ExitStack
import shutil
from typing import Any

import sqlalchemy

from ._util import add_column
from ..database import sql_engine_ctx
from ..tables import schema_8
from .._folder_migrator import Migration
Expand Down Expand Up @@ -41,16 +41,6 @@ def migrate(self, source_dir: Path, dest_dir: Path) -> None:

dest_transaction = exit_stack.enter_context(dest_engine.begin())

def add_column(
engine: sqlalchemy.engine.Engine,
table_name: str,
column: Any,
) -> None:
column_type = column.type.compile(engine.dialect)
engine.execute(
f"ALTER TABLE {table_name} ADD COLUMN {column.key} {column_type}"
)

add_column(
dest_engine,
schema_8.run_command_table.name,
Expand All @@ -73,9 +63,12 @@ def add_column(
def _add_missing_indexes(dest_transaction: sqlalchemy.engine.Connection) -> None:
# todo(2024-11-20): Probably add the indexes missing from prior migrations here.
# https://opentrons.atlassian.net/browse/EXEC-827
dest_transaction.execute(
"CREATE UNIQUE INDEX ix_run_run_id_command_status_index_in_run ON run_command (run_id, command_status, index_in_run);"
index = next(
index
for index in schema_8.run_command_table.indexes
if index.name == "ix_run_run_id_command_status_index_in_run"
)
index.create(dest_transaction)


def _migrate_command_table_with_new_command_error_col_and_command_status(
Expand Down
Loading