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

feat(datasets): Added the Experimental SafetensorsDataset #898

Merged
merged 29 commits into from
Nov 28, 2024
Merged

feat(datasets): Added the Experimental SafetensorsDataset #898

merged 29 commits into from
Nov 28, 2024

Conversation

MinuraPunchihewa
Copy link
Contributor

@MinuraPunchihewa MinuraPunchihewa commented Oct 18, 2024 •
edited
Loading

Description

This PR adds the SafetensorsDataset to support interactions with tensors stored in files in the Safetensors format.

Fixes #221

Development notes

I have used the PickleDataset as a base for the implementation of this dataset (as it goes about the same manner to access files).

These changes have been tested,

  1. Manually, by running the code locally to load and save tensors from and to Safetensors files.
  2. Via the existing and newly added unit tests.

Checklist

  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the relevant RELEASE.md file
  • Added tests to cover my changes
  • Received approvals from at least half of the TSC (required for adding a new, non-experimental dataset)

@MinuraPunchihewa
added the skeleton for the Safetensors experimental dataset
26f714d 
Signed-off-by: Minura Punchihewa <[email protected]>
@MinuraPunchihewa
implemented the save() and load() funcs
c2a980e 
Signed-off-by: Minura Punchihewa <[email protected]>
@MinuraPunchihewa
updated the default backend
58a258d 
Signed-off-by: Minura Punchihewa <[email protected]>
@MinuraPunchihewa
implemented the describe() and exists() funcs
3723b02 
Signed-off-by: Minura Punchihewa <[email protected]>
@MinuraPunchihewa
imported the dataset to main pkg
843e815 
Signed-off-by: Minura Punchihewa <[email protected]>
@MinuraPunchihewa
fixed how data is passed to load()
9a54930 
Signed-off-by: Minura Punchihewa <[email protected]>
@MinuraPunchihewa
fixed save() to access the file path
8a5d522 
Signed-off-by: Minura Punchihewa <[email protected]>
@MinuraPunchihewa
added a release() func
1bd4117 
Signed-off-by: Minura Punchihewa <[email protected]>
@MinuraPunchihewa
added the docstrings for the dataset
eba18df 
Signed-off-by: Minura Punchihewa <[email protected]>
@MinuraPunchihewa
fixed lint issues
98bb719 
Signed-off-by: Minura Punchihewa <[email protected]>
@MinuraPunchihewa MinuraPunchihewa force-pushed the feature/safetensors_dataset branch from e982ffe to 98bb719 Compare October 19, 2024 19:15
@MinuraPunchihewa MinuraPunchihewa marked this pull request as ready for review October 20, 2024 06:39
@MinuraPunchihewa
Copy link
Contributor Author

Hey @astrojuanlu,
This is my implementation of the SafetensorsDataset. I would appreciate a review.

@MinuraPunchihewa
fixed lint issues
aa6dd9c 
Signed-off-by: Minura Punchihewa <[email protected]>
@MinuraPunchihewa
fixed use of insecure temp files
697f34f 
Signed-off-by: Minura Punchihewa <[email protected]>
@MinuraPunchihewa
added the dataset to the documentation
4c27e73 
Signed-off-by: Minura Punchihewa <[email protected]>
@MinuraPunchihewa
listed the dependencies for the dataset
7922ed8 
Signed-off-by: Minura Punchihewa <[email protected]>
@MinuraPunchihewa
fixed typo in dataset reference
3c59813 
Signed-off-by: Minura Punchihewa <[email protected]>
@DimedS DimedS added the Community Issue/PR opened by the open-source community label Nov 8, 2024
DimedS
DimedS approved these changes Nov 12, 2024
View reviewed changes
Copy link
Member

@DimedS DimedS left a comment

Choose a reason for hiding this comment

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

Thank you for the PR, @MinuraPunchihewa! It looks great to me. I tested it manually, and everything works perfectly!

@MinuraPunchihewa
Copy link
Contributor Author

Thank you for the PR, @MinuraPunchihewa! It looks great to me. I tested it manually, and everything works perfectly!

Thank you, @DimedS. I appreciate it. I just updated the release notes as well.

astrojuanlu
astrojuanlu approved these changes Nov 25, 2024
View reviewed changes
Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

Apart from the comments I made on dependencies and defaults, I tested this with local and S3 URLs, saving and loading data, loading data saved with one backend from another backend. It all seems to work. Thanks @MinuraPunchihewa !

@merelcht merelcht mentioned this pull request Nov 25, 2024
Closed
7 tasks
MinuraPunchihewa and others added 7 commits November 27, 2024 22:42
@astrojuanlu
Copy link
Member

This is going in, thanks @MinuraPunchihewa again!

@astrojuanlu astrojuanlu merged commit 0dec688 into kedro-org:main Nov 28, 2024
12 checks passed
@MinuraPunchihewa
Copy link
Contributor Author

This is going in, thanks @MinuraPunchihewa again!

It's a pleasure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noklam noklam noklam left review comments

@astrojuanlu astrojuanlu astrojuanlu approved these changes

@DimedS DimedS DimedS approved these changes

@ElenaKhaustova ElenaKhaustova Awaiting requested review from ElenaKhaustova

Assignees
No one assigned
Labels
Community Issue/PR opened by the open-source community
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add safetensors dataset
4 participants
@MinuraPunchihewa @astrojuanlu @noklam @DimedS