Skip to content
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

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bdpedigo
Copy link
Collaborator

Related to CAVEconnectome/MaterializationEngine#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
Copy link
Collaborator Author

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
Copy link
Collaborator

ceesem commented Mar 21, 2024

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
Copy link
Collaborator Author

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
Copy link
Collaborator

ceesem commented Mar 21, 2024

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@fcollman
Copy link
Collaborator

@bdpedigo do we want to revist this PR?

@bdpedigo
Copy link
Collaborator Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants