search

CAVEconnectome / CAVEclient

This is the python client for accessing REST APIs within the Connectome Annotation Versioning Engine.
https://caveconnectome.github.io/CAVEclient/
MIT License
19 stars 12 forks source link

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

Open bdpedigo opened 3 months ago

bdpedigo commented 3 months ago

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 commented 3 months ago

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

ceesem commented 3 months ago

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

bdpedigo commented 3 months ago

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?

ceesem commented 3 months ago

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.

bdpedigo commented 3 months ago

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 commented 3 months ago

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

fcollman commented 2 months ago

@bdpedigo do we want to revist this PR?

bdpedigo commented 2 months ago

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?