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

Raising RuntimeErrors when datasets missing #2430

Closed
wants to merge 3 commits into from

Conversation

skim0514
Copy link
Contributor

@skim0514 skim0514 commented Jun 1, 2022

Checks download flag and raises error when dataset is missing given download flag exists. Unit tested manually.

edit: Changed path to check as well as comment that is returned.

@skim0514 skim0514 changed the title raising RuntimeErrors when datasets missing Raising RuntimeErrors when datasets missing Jun 1, 2022
@@ -120,7 +120,12 @@ def __init__(
checksum = _CHECKSUMS.get(url, None)
download_url_to_file(url, archive, hash_prefix=checksum)
extract_archive(archive)

else:
if not os.path.exists(archive):
Copy link
Member

Choose a reason for hiding this comment

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

Here it assumes download is False, and archive is not found. If users extracted the files to self._path and deleted the archive, it should not throw a runtime error.

We can change the archive to self._path as a workaround.

Comment on lines 126 to 127
"The file is not found in the following location. "
f"Set `download=True` to download it. {archive}"
Copy link
Member

Choose a reason for hiding this comment

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

It can be rephrased like this:

Suggested change
"The file is not found in the following location. "
f"Set `download=True` to download it. {archive}"
f"The path {self._path} doesn't exist."
"Please check the ``root`` path or set `download=True` to download it"

Copy link
Member

@nateanl nateanl left a comment

Choose a reason for hiding this comment

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

LGTM

@facebook-github-bot
Copy link
Contributor

@skim0514 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants