-
Notifications
You must be signed in to change notification settings - Fork 202
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
Fix IBL streaming tests #3718
base: main
Are you sure you want to change the base?
Fix IBL streaming tests #3718
Conversation
@@ -198,9 +198,9 @@ def test_ibl_sorting_extractor(self): | |||
except: | |||
pytest.skip("Skipping test due to server being down.") | |||
sorting = read_ibl_sorting(pid=PID, one=one) | |||
assert len(sorting.unit_ids) == 733 | |||
assert len(sorting.unit_ids) == 1091 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems really risky to have a gt data set and then just test that we load that dataset. Especially if they can change. When I was looking at the failed tests it also said a failure between subtracting datetime.datetime and NoneType. Is that error related to this too? Seems like changes in IBL overall no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah maybe we can pin it to a previous version? I agree that probably we don't need to assert the number of units. Maybe just asserting that good units are less than all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems safer for a test from my perspective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I can show you how to use the revision
argument that pins the query to a version of the dataset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks that would be great!
Hi Alessio, |
implement the revision argument to the IBL loader
for more information, see https://pre-commit.ci
@zm711 I think that after adding Reayd to merge on my end |
Sounds good to me! |
And this should fix the datetime issue too? |
No :( @oliche can you help us debug this error as well?
|
Somehow the same PID resulted in a different number of units.
@oliche anything changed on the ONE-api side? Is it possible that a PID is now pointing to a different insertion?