-
Notifications
You must be signed in to change notification settings - Fork 932
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
Add CSV Reader options classes to pylibcudf #17412
Conversation
python/cudf/cudf/_lib/csv.pyx
Outdated
options.set_prefix(prefix) | ||
|
||
if usecols is not None: | ||
if all([isinstance(col, int) for col in usecols]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if all([isinstance(col, int) for col in usecols]): | |
if all(isinstance(col, int) for col in usecols): |
@@ -54,7 +96,7 @@ def read_csv( | |||
# detect_whitespace_around_quotes: bool = False, | |||
# timestamp_type: DataType = DataType(type_id.EMPTY), | |||
) -> TableWithMetadata: ... | |||
def write_csv(options: CsvWriterOptionsBuilder) -> None: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this annotation incorrect before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I don't think so. I wanted to stay consistent because I noticed we dropped the None
in other PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. IMO we should still include None
as the output type cc @wence-
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll follow up based on what we decide
@@ -1,6 +1,7 @@ | |||
# Copyright (c) 2024, NVIDIA CORPORATION. | |||
|
|||
from collections.abc import Mapping | |||
from typing import Self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since cudf still supports Python 3.10, we'll need to import this from typing_extensions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The None
annotation question is non-blocking for me
/merge |
Description
This PR adds the CSV reader options classes to pylibcudf and plumbs the changes through cudf python.
Checklist