dagster icon indicating copy to clipboard operation
dagster copied to clipboard

:sparkles: merge dagster-polars library

Open danielgafni opened this issue 1 year ago • 6 comments

Summary & Motivation

This PR merges dagster-polars, which has been maintained in a separate repo, in the main Dagster project.

We agreed on this with @PedramNavid.

How I Tested These Changes

The PR includes tests

danielgafni avatar Dec 29 '23 10:12 danielgafni

The docs are not ready yet, will do next week

danielgafni avatar Dec 29 '23 10:12 danielgafni

Thanks for submitting this @danielgafni !

Few early questions:

  • Could you remove the changes to unrelated docs from this commit? We prefer to keep our commits isolated and self-contained as much as possible.
  • Could you remove the files that are fully commented-out?

That will help make this easier for review! Thank you

PedramNavid avatar Dec 29 '23 20:12 PedramNavid

Hey, sure!

However, I'll have to disable pre-commit in this case. I mentioned these edits were added by pre-commit automatically. This happens quite often with my Dagster PRs, I think that's because pre-commit is not being enforced by CI, which results in some commits being merged to master without running it beforehand.

Another option would be running the hooks right now in master.

A better long-term solution would be to enforce pre-commit checks in CI, otherwise one can't turn them on locally.

As for the code - for now I simply copied it from my repo without noticeable changes. We can definitely do a cleanup tho (I think there is just 1 commented tests files). One more thing I wanted to propose was moving some of the nice IOManager features I have to the UPathIOManager.

danielgafni avatar Dec 29 '23 20:12 danielgafni

ok removed pre-commit edits!

danielgafni avatar Dec 29 '23 20:12 danielgafni

Hey @PedramNavid , I'm done with the API docs. Would you be able to take a look at them, please?

Meanwhile I'll finish the tutorial.

danielgafni avatar Jan 11 '24 14:01 danielgafni

@PedramNavid @erinkcochran87 I believe I'm done!

danielgafni avatar Jan 14 '24 21:01 danielgafni