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.

Sign up for GitHub

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

Jump to bottom

Update polars to fix PanicException #2585

Merged
merged 13 commits into from
Mar 14, 2024
Merged

Update polars to fix PanicException #2585

merged 13 commits into from
Mar 14, 2024

Conversation

polinaeterna
Copy link
Contributor

@polinaeterna polinaeterna commented Mar 13, 2024 •
edited
Loading

should fix the viewer for https://huggingface.co/datasets/teknium/OpenHermes-2.5 and presumably many others

There is and issue for this in polars: pola-rs/polars#3942.

I didn't understand though why some cases with nested structs work while some like this doesn't

@polinaeterna polinaeterna marked this pull request as draft March 13, 2024 15:14
@polinaeterna polinaeterna marked this pull request as ready for review March 13, 2024 17:00
@polinaeterna polinaeterna requested a review from severo March 13, 2024 17:00
@polinaeterna
Copy link
Contributor Author

i wanted to add a testcase for this but i don't understand what's different comparing to what we already have

severo
severo approved these changes Mar 14, 2024
View reviewed changes
Copy link
Collaborator

@severo severo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my review is somewhat superficial, because I don't know the code well, and we could not reproduce the issue with a unit test...
Could you test on a specific dataset and see if it fixed the issue?

services/worker/src/worker/job_runners/split/descriptive_statistics.py Show resolved Hide resolved
@polinaeterna polinaeterna requested a review from severo March 14, 2024 15:01
@polinaeterna
Copy link
Contributor Author

@severo i added a test with a real file 452fbcd

i checked on the CI with an old version of polars that it doesn't pass:
image
and now i passes

severo
severo approved these changes Mar 14, 2024
View reviewed changes
services/worker/tests/job_runners/split/test_descriptive_statistics.py Outdated

def test_polars_struct_thread_panic_error(struct_thread_panic_error_parquet_file: str) -> None:
from polars import Float64, List, Struct
from polars import Utf8 as String # string type in polars <0.20 is called Utf8, in 0.20 it's an alias to String
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we use 0.20, we can just import String, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes it was just to check that 0.19 fails. should i do just from polars import String?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. I understand that it was for 0.19, but now that we use 0.20, it's not needed anymore, right?

@polinaeterna polinaeterna merged commit f2e3c63 into main Mar 14, 2024
6 checks passed
@polinaeterna polinaeterna deleted the update-polars branch March 14, 2024 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@severo severo severo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@polinaeterna @severo