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

fix: update LDF.collect() for polars==0.19.8 #24

Closed
wants to merge 1 commit into from

Conversation

brendancooley
Copy link
Contributor

polars removed **kwargs from LazyFrame.collect() in the 0.19.8 release. Since the signature for patito's child LDF.collect does not match that of pl.LazyFrame.collect, the test_dummy_data suite fails after upgrading polars. This PR moves collect to an *args, **kwargs pattern for better backward/forward compatibility. Tests pass with polars==0.19.8 and polars==0.18.7.

fix: switch to *args/**kwargs
@ion-elgreco
Copy link
Contributor

Thanks I also ran into the issue, I've merged this fix into my own develop branch of patito fork. There isn't much activity in this repo anymore it seems.

@thomasaarholt
Copy link
Collaborator

We're working on a migration to pydantic v2, it's just difficult to squeeze in development time between our jobs.

@ion-elgreco
Copy link
Contributor

We're working on a migration to pydantic v2, it's just difficult to squeeze in development time between our jobs.

Wouldn't it then make sense to at least merge all the fixes, so the current release works as expected?

@thomasaarholt
Copy link
Collaborator

We have a work-in-progress branch that we are migrating the pydantic work with, and it is extensive enough that we didn't want to risk having to do rebases while still doing that. I agree that the fixes here are valuable, but we decided to prioritize the pydantic migration first.

@ion-elgreco
Copy link
Contributor

We have a work-in-progress branch that we are migrating the pydantic work with, and it is extensive enough that we didn't want to risk having to do rebases while still doing that. I agree that the fixes here are valuable, but we decided to prioritize the pydantic migration first.

Are all the current issues accounted for in the Pydantic v2 release or will the same issues persist?

@thomasaarholt
Copy link
Collaborator

We will likely have fixed some issues, while others will persist. We will rebase the PRs on the pydantic-migrated-version and then go through the PRs one by one.

@ion-elgreco
Copy link
Contributor

We will likely have fixed some issues, while others will persist. We will rebase the PRs on the pydantic-migrated-version and then go through the PRs one by one.

If you need any help to port the fixes to the pydantic-migrated version, I am happy to help there. We use Patito quite extensively in our codebase

@thomasaarholt
Copy link
Collaborator

This is now fixed in the main branch :)

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