CAVEclient
CAVEclient copied to clipboard
Protect against silent error in root IDs with `select_columns` and `timestamp`
Related to https://github.com/CAVEconnectome/MaterializationEngine/issues/134
This just raises an error client-side and instructs the user how to temporarily fix.
In the future this will be fixed server-side, but still worth having this here in case someone is on an older version of materialization engine.
control logic between query_table
/live_query
and the value of timestamp
gets a bit complicated so I saw a few places in the code this could have gone, happy to discuss
I don't think this is a good solution philosophically. If we are going to do a workaround at the client level, I think we should auto-insert X_supervoxel_id
if X_root_id
is also in the selected columns and I guess strip it back out again on return, but that's also the fix to be done on the server...
i think the concern is even once we fix server side as you say, someone still could be running an older version of the engine server side and would still have this issue. happy to brainstorm a better solution. im not against the auto insert/strip approach client-side, personally, since that is essentially what this fix asks you to do on your own. thoughts @fcollman?
As an aside, I think we need to standardize formatting again — this PR has a lot of formatting changes in addition to the code changes. We had previously landed on black before, but it seems like ruff is behaving somewhat differently. Certainly on import order, but also some multiline things.
very much agree with the sentiment to standardize formatting, but don't think it is ruff vs black. most recent black does lots of changes:
actually for import order you might be right, i dont think black touches that if i recall correctly
@bdpedigo do we want to revist this PR?
sure, i got a little lost in what the preferred (if temporary) fix would ideally be client side. we talked about
- throwing an error if not using supervoxel_id and requesting root IDs (this PR) and using a timestamp (current state of this PR)
- just inserting those columns and removing them later, until there's a server-side fix that does the same, or
- something else?
thoughts?