Skip to content

[REVIEW] Reduce/Remove reliance on **kwargs and *args in IO readers & writers - #12025

Merged
rapids-bot[bot] merged 52 commits into
NVIDIA:branch-22.12from
galipremsagar:11780
Nov 1, 2022
Merged

[REVIEW] Reduce/Remove reliance on **kwargs and *args in IO readers & writers#12025
rapids-bot[bot] merged 52 commits into
NVIDIA:branch-22.12from
galipremsagar:11780

Conversation

@galipremsagar

Copy link
Copy Markdown
Contributor

Description

Resolves: #11780

This PR:

  • Reduces reliance on args & kwargs for readers and writers when cudf engine is selected. However, these will have to stay for the purpose of other engines we support in few readers & writers such as pandas & pyarrow engines.
  • Fixes some bugs where dead parameters were still being used.
  • Fixes some bugs where parameters weren't being passed until the cython later in the first place.
  • Updates docs related to newly exposed parameters.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@galipremsagar galipremsagar added bug Something isn't working 3 - Ready for Review Ready for review by team Python Affects Python cuDF API. 4 - Needs cuDF (Python) Reviewer breaking Breaking change labels Oct 28, 2022
@galipremsagar

galipremsagar commented Oct 31, 2022

Copy link
Copy Markdown
Contributor Author

@bdice Should be ready for another round of review.

@galipremsagar
galipremsagar requested a review from bdice October 31, 2022 20:25
@ayushdg

ayushdg commented Oct 31, 2022

Copy link
Copy Markdown
Contributor

This pr also fixes #6916

@galipremsagar galipremsagar linked an issue Oct 31, 2022 that may be closed by this pull request
Comment thread python/cudf/cudf/_lib/parquet.pyx Outdated
Comment thread python/cudf/cudf/utils/ioutils.py Outdated

@wence- wence- left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Two very minor niggles, but otherwise looks great, thanks for this!

@bdice bdice left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A few minor suggestions, otherwise LGTM. Thank you for the iterations!

Comment thread python/cudf/cudf/io/parquet.py Outdated
Comment thread python/cudf/cudf/utils/ioutils.py Outdated
Comment thread python/cudf/cudf/utils/ioutils.py Outdated
Comment thread python/cudf/cudf/utils/ioutils.py Outdated
Comment thread python/cudf/cudf/utils/ioutils.py Outdated
galipremsagar and others added 2 commits November 1, 2022 08:26
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
@galipremsagar galipremsagar added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 4 - Needs cuDF (Python) Reviewer 3 - Ready for Review Ready for review by team labels Nov 1, 2022
@galipremsagar

Copy link
Copy Markdown
Contributor Author

@gpucibot merge

@rapids-bot
rapids-bot Bot merged commit d236779 into NVIDIA:branch-22.12 Nov 1, 2022
@galipremsagar

Copy link
Copy Markdown
Contributor Author

Thanks for patiently reviewing this PR @bdice & @wence-, I know this was a tedious one.

rapids-bot Bot pushed a commit that referenced this pull request Nov 1, 2022
This PR removes all "smart quotes" from the library by enforcing a pre-commit hook.

Smart quotes typically arise from copying rendered docstrings from Pandas, because Sphinx automatically transforms straight quotes into smart quotes when rendering the docs as HTML. However, the use of smart quotes is undesirable in code, and makes it difficult to do find-replace transformations if straight and smart quotes are mixed.

I have made suggestions to fix this several times before, so I am making the suggestions more permanent and automatically enforceable via a pre-commit style check:
- #12025 (comment)
- #9817 (comment)
- #9571 (comment)

Authors:
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - GALI PREM SAGAR (https://github.com/galipremsagar)

URL: #12035
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

5 - Ready to Merge Testing and reviews complete, ready to merge breaking Breaking change bug Something isn't working Python Affects Python cuDF API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEA] Remove **kwargs in all IO readers and writers [FEA] Expose storage_options kwarg to readers/writers

4 participants