feat(robot-server): handle new offset locations in /labwareOffsets #17388
Conversation
sfoster1
requested review from
SyntaxColoring,
TamarZanzouri,
smb2268 and
mjhuff
[pounding knife and fork on table] CTE! CTE! CTE!
sfoster1
force-pushed
the
exec-1105-server-integration
branch
from
d715f4c to
b92e843
Compare
There was a problem hiding this comment.
That's some fun SQL. Thank you for doing this! I did test it on a robot and can confirm it works, see
migration_test.json
Couple Qs for my own edifcation.
There was a problem hiding this comment.
Just curious, do we create any kind of db backup when performing migrations?
There was a problem hiding this comment.
Yes, or rather, we leave the old one. Before the code in this file runs, we actually make a copy of the entire persistence directory (including the database file) to a new directory, so this operation isn't actually touching the pre-migration database.
| def do_active_filter(self, active: bool) -> Self: | ||
| """Filter to only active=active rows.""" | ||
| self._current_base_filter_statement = self._current_base_filter_statement.where( | ||
| labware_offset_table.c.active == True # noqa: E712 |
There was a problem hiding this comment.
Drive-by comment: What is the point of the active argument then?
And what is the docstring referring to when it says only active=active rows?
There was a problem hiding this comment.
in the comment, the first active is the column name and the second active is the argument name. could stand to be a little clearer - it's saying it filters to only rows whose active column matches the parameter.
Uhhh and that looks like a typo that it filters it to c.active == True instead of c.active == active
There was a problem hiding this comment.
Thanks!
I think we should take a hard look at whether we really need each location component to be its own row. It seems like it adds a lot of complexity, and I don't see what it gets us in return. In the same vein, also think we should consider overwriting the existing schema 9 instead of preserving it in the migration chain.
Besides that, various small things.
| ) | ||
|
|
||
| labware_offset_location_sequence_components_table = sqlalchemy.Table( | ||
| "labware_offset_sequence_components", |
There was a problem hiding this comment.
Nitpick: theoretically, our naming convention for table names is singular (labware_offset_sequence_component).
This hasn't been followed well in all of our tables and I don't know if it's a battle worth fighting.
| else: | ||
| # This should never happen since we're exhaustively checking kinds here | ||
| continue |
There was a problem hiding this comment.
Can this be typing_extensions.assert_never(sequence_component.kind) instead of continue?
There was a problem hiding this comment.
as long as that doesn't result in a runtime assertion, yes. if it did, then this endpoint would 500 if we ever added a new component and then downmigrated, and i would love for that to not happen
There was a problem hiding this comment.
which i'm realizing I did not explicitly handle and should, oops
Talked about the migration inline, but having the location components be their own rows like this is the way you do things in sql and I'm pretty happy with it - I do not see it as added complexity. We get to push off all the searching and ordering into the system intended to do that, and there's the small cost of collation which is a hundred lines of code that probably won't need to change. |
This will allow us to add new kinds of location without doing schema changes and migrations. If you add a location and downgrade to a version that doesn't understand it, you'll get an "UnknownLocationSequenceComponent", just like a bad run.
sfoster1
force-pushed
the
exec-1105-server-integration
branch
from
78f1172 to
443a8df
Compare
Adds the new format for labware offset locations from #17363 to the HTTP API
/labwareOffsetsendpoint.The endpoint now accepts and returns labware offset locations as these sequences. The filter API is for now unchanged, and still works properly to filter out or in offsets based on their core features or on their locations.
The types are defined separately from the protocol engine because we'll be shortly adding a concept of a "general offset" to this API, and that is best modeled as another heterogenous union entry, which means the types will start to diverge.
This comes along with a sql schema migration; the offset locations are normalized out into an offset table with a foreign key into the main offset table. All content from the offset table is migrated to the new one in the standard persistence directory migration api.
Reviews
Testing