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

Implement Dataset from CSV #1946

Merged
merged 32 commits into from
Mar 12, 2021
Merged

Implement Dataset from CSV #1946

merged 32 commits into from
Mar 12, 2021

Conversation

albertvillanova
Copy link
Member

Implement Dataset.from_csv.

Analogue to #1943.

If finally, the scripts should be used instead, at least we can reuse the tests here.

@albertvillanova albertvillanova marked this pull request as draft February 25, 2021 19:15
albertvillanova
albertvillanova commented Mar 2, 2021
View reviewed changes
src/datasets/io/csv.py Outdated
self,
path,
name=None,
data_dir=None,
Copy link
Member Author

Choose a reason for hiding this comment

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

@lhoestq do we need data_dir for Dataset.from_csv()?

lhoestq
lhoestq reviewed Mar 3, 2021
View reviewed changes
src/datasets/io/abc.py Outdated Show resolved Hide resolved
lhoestq
lhoestq reviewed Mar 4, 2021
View reviewed changes
Copy link
Member

@lhoestq lhoestq left a comment

Choose a reason for hiding this comment

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

Awesome :)

src/datasets/io/csv.py Outdated Show resolved Hide resolved
src/datasets/io/csv.py Outdated Show resolved Hide resolved
@albertvillanova albertvillanova marked this pull request as ready for review March 9, 2021 10:03
lhoestq
lhoestq reviewed Mar 9, 2021
View reviewed changes
Copy link
Member

@lhoestq lhoestq left a comment

Choose a reason for hiding this comment

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

Nice ! 🔥

I think the only thing left to do is just add tests for Dataset.from_csv and DatasetDict.from_csv in test_arrrow_dataset.py and test_dataset_dict.py
Good job !

src/datasets/arrow_dataset.py Show resolved Hide resolved
@albertvillanova
Copy link
Member Author

@lhoestq question about public API: keep_in_memory or just in_memory?

@lhoestq
Copy link
Member

lhoestq commented Mar 10, 2021

For consistence I'd say keep_in_memory, but no strong opinion.

lhoestq
lhoestq approved these changes Mar 10, 2021
View reviewed changes
Copy link
Member

@lhoestq lhoestq left a comment

Choose a reason for hiding this comment

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

Looks all good ! Thanks for adding this 🎉

Feel free to also add from_csv to the list of documented methods in the documentation in main_classes.rst.

@albertvillanova
Copy link
Member Author

@lhoestq done!

@albertvillanova albertvillanova mentioned this pull request Mar 11, 2021
Merged
@lhoestq lhoestq merged commit 77bb240 into huggingface:master Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lhoestq lhoestq lhoestq 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.
2 participants
@albertvillanova @lhoestq