CAVEclient icon indicating copy to clipboard operation
CAVEclient copied to clipboard

Protect against silent error in root IDs with `select_columns` and `timestamp`

Open bdpedigo opened this issue 11 months ago • 8 comments

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.

bdpedigo avatar Mar 21 '24 16:03 bdpedigo

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

bdpedigo avatar Mar 21 '24 17:03 bdpedigo

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...

ceesem avatar Mar 21 '24 18:03 ceesem

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?

bdpedigo avatar Mar 21 '24 18:03 bdpedigo

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.

ceesem avatar Mar 21 '24 18:03 ceesem

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:

Screenshot 2024-03-21 at 11 44 03 AM

bdpedigo avatar Mar 21 '24 18:03 bdpedigo

actually for import order you might be right, i dont think black touches that if i recall correctly

bdpedigo avatar Mar 21 '24 18:03 bdpedigo

@bdpedigo do we want to revist this PR?

fcollman avatar Apr 26 '24 16:04 fcollman

sure, i got a little lost in what the preferred (if temporary) fix would ideally be client side. we talked about

  1. throwing an error if not using supervoxel_id and requesting root IDs (this PR) and using a timestamp (current state of this PR)
  2. just inserting those columns and removing them later, until there's a server-side fix that does the same, or
  3. something else?

thoughts?

bdpedigo avatar Apr 26 '24 16:04 bdpedigo