JSON Decoder Error Fix #346
Conversation
facebook-github-bot
added
the
CLA Signed
vissl/utils/io.py
Outdated
| @@ -51,7 +51,7 @@ def create_file_symlink(file1, file2): | |||
| logging.info(f"Could NOT create symlink. Error: {e}") | |||
|
|
|||
|
|
|||
| def save_file(data, filename): | |||
| def save_file(data, filename,rewrite=True): | |||
There was a problem hiding this comment.
So since we do more overwriting vs. appending, instead of rewrite=True, I would use append=False.
- Add option append, with default False to #save_file.
- Add in code to support json append=True and append=False.
- Change here to use append=True. This is the only use of #save_file in the codebase using json format. Global search save_file to validate this.
CC: @prigoyal What do you think?
There was a problem hiding this comment.
I like the idea of append instead of rewrite. All your recommendations make sense. A few additional thoughts:
- let's update the docstring of this function to mention clearly the
appendis applicable only for thejsonformat. - for the above reason, we might want to pick a more verbose name
append_to_jsonfor instance.
There was a problem hiding this comment.
Thanks for feedback @iseessel and @prigoyal ! I have addressed your inputs in e9efe76.
- Switched the variable to
append_to_jsonand set the default value to false. - Updated the associated doc-string of the save_file function to inform the user about the same.
Do let me know if further changes are to be made to polish the PR!
vissl/utils/io.py
Outdated
| """ | ||
| Common i/o utility to handle saving data to various file formats. | ||
| Supported: | ||
| .pkl, .pickle, .npy, .json | ||
| Specifcally for .json, users have the option to eithier append |
There was a problem hiding this comment.
nit: Specifically
nit: either
There was a problem hiding this comment.
With 896a74b, I have fixed the typos and changed the default to True, as it was rewriting in a lot of cases when it should have been appending. This means that the associated tutorials need to be updated as well either to set append_to_json=False or to at least add a comment in the notebook stating that re-running the cell would throw a JSON Decoder error.
GETTING_STARTED.md
Outdated
| @@ -72,7 +72,7 @@ In your python interpretor: | |||
| } | |||
| } | |||
| >>> from vissl.utils.io import save_file | |||
| >>> save_file(json_data, "/tmp/configs/config/dataset_catalog.json") | |||
| >>> save_file(json_data, "/tmp/configs/config/dataset_catalog.json",append_to_json=True) | |||
There was a problem hiding this comment.
These should now be append_to_json=False, since we want to prevent the JSON decoder error.
Also nit: add space after comma:
>>> save_file(json_data, "/tmp/configs/config/dataset_catalog.json", append_to_json=False)
(and for all the others).
|
@iseessel has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Summary: With respect to #342, re-running the cell won't cause JSONDecoderError. Added option for the user if they want to re-write the JSON file or not. Default set to True. CC: iseessel Pull Request resolved: #346 Reviewed By: prigoyal Differential Revision: D29271891 Pulled By: iseessel fbshipit-source-id: 3146f5aa4f47fa9da6c4ecd8237d8739055638d6
With respect to #342, re-running the cell won't cause JSONDecoderError.
Added option for the user if they want to re-write the JSON file or not.
Default set to True.
CC: @iseessel