Fixed dataset download re-linking.#1989
Merged
rpartsey merged 4 commits intofacebookresearch:mainfrom Feb 17, 2023
Merged
Conversation
facebook-github-bot
added
the
CLA Signed
vincentpierre
approved these changes
Jan 19, 2023
| print(f"Note, {default_data_path} is a symbolic link that points to {data_path}.") | ||
|
|
||
| try: | ||
| os.makedirs(data_path, exist_ok=True) |
Contributor
There was a problem hiding this comment.
What kind of errors can arise here? I think it would just be permission errors?
What is the advantage of printing these messages over just raising the error?
Contributor
Author
There was a problem hiding this comment.
To be honest, I didn't ask myself these questions during fixing, but it seems to me that this is a "graceful way" to exit a program. Exceptions should be raised on program errors. Here everything is fine, user specified wrong path and we inform him about it. Why the program should fail with exception?
Contributor
There was a problem hiding this comment.
@rpartsey probably should still be restricted to IOException? LIke no reason to catch SystemError exceptions from Python etc.
0mdc
approved these changes
Jan 26, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation and Context
Assume you already have all the assets downloaded.
When the habitat-sim data download utility is run again for the same type of assets without specifying '--data-path' argument, the
data_pathvariable defaults todata_path = os.path.abspath("./data/")andos.path.exists(data_path)is True for a data symlink. Code checks whether the assets exist the same wayos.path.exists(data_path+path_to_asset)and command also returns True and all of the assets are accessible via symlink (note that at this step some of the assets in /path/to/data may be symlinks but all will have"/path/to/data"as their prefix (as expected)). However, then the symlinks to the assets are unlinked and linked back but with "/path/to/habitat-lab/data" .All this is caused because at the beginning, this command
os.path.abspath("./data/")treats data not as symlink but as actual path (directory). More context in this Slack thread.How Has This Been Tested
Types of changes
Checklist