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.

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

fix(datasets): Use put() and get() instead of copy in TensorFlowModelDataset's _save and _load methods. #844

Merged
merged 4 commits into from
Sep 26, 2024

Conversation

gitgud5000
Copy link
Contributor

@gitgud5000 gitgud5000 commented Sep 22, 2024

Description

This PR resolves the issue in #839, where saving TensorFlowModelDataset to Azure Blob Storage fails due to incorrect use of .copy() from the fsspec.filesystem interface.

Development notes

The issue occurs in the _save and _load methods on lines 147 and 172, where .copy() is used. According to the fsspec documentation, .copy() is intended for remote-to-remote transfers. Since this involves copying from a local filesystem to remote storage, .put() should be used for saving and .get() for loading.

The code has been updated to use .put() and .get() accordingly, replacing the use of .copy().

Both methods work for local-to-local and local-to-remote(& vice versa) transfers based on testing.

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

…owModelDataset`'s `_save` and `_load` methods.

Signed-off-by: gitgud5000 <17186026+gitgud5000@users.noreply.github.com>
Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

Thanks for the investigation and contribution of the fix @gitgud5000 ! This looks good to me 👍

Can you add a comment in the release notes and add your name to the contributors list for the upcoming release as well? 🙂

Copy link
Contributor

@ankatiyar ankatiyar left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @gitgud5000! ⭐

Signed-off-by: gitgud5000 <17186026+gitgud5000@users.noreply.github.com>
Signed-off-by: Ankita Katiyar <110245118+ankatiyar@users.noreply.github.com>
@ankatiyar ankatiyar enabled auto-merge (squash) September 26, 2024 09:16
@ankatiyar ankatiyar merged commit 0a3a381 into kedro-org:main Sep 26, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants