Update Error message for VersionNotFoundError to handle Permission related issues better

[ankatiyar]

Signed-off-by: Ankita Katiyar [email protected]

Description

Resolves #1768

Development notes

Fresh PR to resolve the issue.
Closing #1809 in favour of this solution - PermissionError was being handled outside of Kedro in some cases. This PR is a workaround as the error message is insufficient and it's hard to pinpoint the source of error precisely in case of cloud storage.

Proposed Solution

As a result, this PR modifies the error message for VersionNotFoundError to indicate that this error could be happening due to insufficient permission in case of cloud storage i.e. S3, GCS etc.

Checklist

• Read the contributing guidelines
• 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

[noklam]

@ankatiyar Don't forget to document how this decision was made!

[noklam]

Suggested change

	def test_no_version_cloud(self):
	def test_no_versions_with_cloud_protocol(self):

Just keeping it consistent as the other codebase use test_no_versions

[ankatiyar]

Made the change in 211c449

[noklam]

Suggested change

	ds = CSVDataSet("s3://bucket/file.csv", version=version)
	versioned_dataset = CSVDataSet("s3://bucket/file.csv", version=version)

[ankatiyar]

Made the change in 211c449

[noklam]

    def test_no_versions(self, versioned_plot_writer):
        """Check the error if no versions are available for load."""
        pattern = r"Did not find any versions for MatplotlibWriter\(.+\)"
        with pytest.raises(DataSetError, match=pattern):
            versioned_plot_writer.load()

I found this test in one of our datasets test, can we follow the same pattern here? I think "rf" should be the same as escape.

[ankatiyar]

I tried using "rf" but it doesn't seem to work.

[noklam]

Thank you for sticking with it, I think this is a tough one as the first few issues. It's in a good shape already, most of my comments are around the styling.

A good tip is always check the existing codebase and see if there are something similar exist.

[merelcht]

Just to double check that I understand this correctly: the error returned by fsspec isn't a PermissionError and that's why we cannot know for sure that there is something wrong with permission?

[merelcht]

Suggested change

	f"Did not find any versions for {self} This could be "
	f"Did not find any versions for {self}. This could be "

[noklam]

@MerelTheisenQB Correct, it's possible to get an error with some other functions , but all datasets rely on self._fs.glob and it won't give you any error with permission issues.

It seems that this is the expected behavior of glob. For example, if you actually ls the system, you will get a permission error. However, this is a very expensive operation especially with object stores, thus I didn't like it.

[ankatiyar]

Made the change in 211c449

[merelcht]

Thanks for the explanation @noklam 🙂

[merelcht]

This was definitely a lot trickier than we thought, but this solution makes sense to me 👍

Don't forget to add the change in the release notes.

[AhdraMeraliQB]

Nice work on this!! Thanks for sticking with it 😄