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

FIX-#6097: Pass storage_options to the to_csv function of PandasOnRay class with fsspec #6098

Merged
merged 1 commit into from
May 9, 2023
Merged

FIX-#6097: Pass storage_options to the to_csv function of PandasOnRay class with fsspec #6098

merged 1 commit into from
May 9, 2023

Conversation

datth4
Copy link
Contributor

@datth4 datth4 commented May 8, 2023 •
edited
Loading

…IO class with fsspec

What do these changes do?

  • first commit message and PR title follow format outlined here

    NOTE: If you edit the PR title to match this format, you need to add another commit (even if it's empty) or amend your last commit for the CI job that checks the PR title to pick up the new PR title.

  • passes flake8 modin/ asv_bench/benchmarks scripts/doc_checker.py
  • passes black --check modin/ asv_bench/benchmarks scripts/doc_checker.py
  • signed commit with git commit -s
  • Resolves BUG: Failed to pass storage_options parameter to the to_csv function of PandasOnRayIO class with fsspec #6097
  • tests added and passing
  • module layout described at docs/development/architecture.rst is up-to-date

@datth4 datth4 requested a review from a team as a code owner May 8, 2023 06:21
@RehanSD
Copy link
Collaborator

RehanSD commented May 8, 2023

Hi @datth4! Thank you for opening this PR! The fix looks good to me!

RehanSD
RehanSD approved these changes May 8, 2023
View reviewed changes
Copy link
Collaborator

@RehanSD RehanSD left a comment

Choose a reason for hiding this comment

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

LGTM!

@RehanSD
Copy link
Collaborator

RehanSD commented May 8, 2023

@datth4 do you mind pushing a commit that has been git signed off? That's one of the requirements for our PR's to be merged! Thank you so much for opening this PR! The fix looks great!

@datth4
Copy link
Contributor Author

datth4 commented May 9, 2023 •
edited
Loading

Hi @RehanSD , I already signed off the commit, please check it again. Thank you so much.
Screenshot 2023-05-09 at 10 11 17

@RehanSD
Copy link
Collaborator

RehanSD commented May 9, 2023

Hi @datth4! I noticed that the commit is GPG signed, but would you be able to GPG sign it + sign it off with git? (That adds a Signed off by: <YOUR_NAME> <YOUR_EMAIL> to your commit). It requires the -s (lowercase) tag in addition to the GPG Signing -S (uppercase) tag! https://docs.pi-hole.net/guides/github/how-to-signoff/

@RehanSD
Copy link
Collaborator

RehanSD commented May 9, 2023

Thank you for the great work @datth4! Merging!

@RehanSD RehanSD merged commit 632d724 into modin-project:master May 9, 2023
YarShev
YarShev reviewed May 15, 2023
View reviewed changes
modin/core/execution/ray/implementations/pandas_on_ray/io/io.py
Comment on lines +163 to +165
storage_options = csv_kwargs.pop("storage_options", None)
df.to_csv(**csv_kwargs)
csv_kwargs.update({"storage_options": storage_options})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same things should be done for PandasOnUnidist. @arunjose696, can you open a PR for that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@arunjose696, please wait when we figure out the problem.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@arunjose696, please go ahead with #6172.

@YarShev
Copy link
Collaborator

YarShev commented May 19, 2023

@RehanSD, I am still seeing the issue on master. Why were these changes merged? There is no even a test.

@datth4, aren't you experiencing the issue with these changes?

@datth4
Copy link
Contributor Author

datth4 commented May 19, 2023 •
edited
Loading

@YarShev, it works for me.
Screenshot 2023-05-19 at 17 08 08

@YarShev
Copy link
Collaborator

YarShev commented May 19, 2023

@datth4, @RehanSD, sorry, my bad, tested other things.

@arunjose696 arunjose696 mentioned this pull request May 19, 2023
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YarShev YarShev YarShev left review comments

@RehanSD RehanSD RehanSD 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.

BUG: Failed to pass storage_options parameter to the to_csv function of PandasOnRayIO class with fsspec
3 participants
@datth4 @RehanSD @YarShev