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: implement _exists for SharedMemoryDataset #4121

Conversation

felixscherz
Copy link
Contributor

@felixscherz felixscherz commented Aug 27, 2024

Description

This adds an _exists method to the SharedMemoryDataset that calls the underlying MemoryDataset instead of relying on the inherited AbstractDataset._exists method.
This is intended to fix #3703

Development notes

Implemented _exists for SharedMemoryDataset and added a test to verify that SharedMemoryDataset.exists accurately reflects the state of the underlying MemoryDataset.

Checklist

  • Read the contributing guidelines
  • Signed off each commit with a Developer Certificate of Origin (DCO)
  • 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 RELEASE.md file
  • Added tests to cover my changes
  • Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

Signed-off-by: Felix Scherz <felixwscherz@gmail.com>
Signed-off-by: Felix Scherz <felixwscherz@gmail.com>
Signed-off-by: Felix Scherz <felixwscherz@gmail.com>
@felixscherz felixscherz force-pushed the fix/exists-method-for-shared-memory-dataset branch from a97a74e to caa5b1d Compare September 10, 2024 11:39
Signed-off-by: Ankita Katiyar <110245118+ankatiyar@users.noreply.github.com>
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.

Thank you @felixscherz for this contribution, this fix makes sense to me!

Copy link
Contributor

@ElenaKhaustova ElenaKhaustova 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, @felixscherz!

This fix makes sense to me to avoid warning.

Though I'm not sure whether we can rely on the state of SharedMemoryDataset, I think we can merge this fix and later re-consider the purpose of exists for SharedMemoryDataset as well as it's purpose (exists) overall during data catalog redesign work: #3995

@ankatiyar ankatiyar merged commit 7537eae into kedro-org:main Sep 24, 2024
39 of 40 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.

ShareMemoryDataset does not have exists() method
3 participants