Fix json WindowsPath crash #8662
Conversation
On Windows, os.path.join returns a WindowsPath. to_json_string does not convert this from a WindowsPath to a string. Added check for WindowsPath to to_json_saveable.
…fix-json-windows-path-crash
|
Thanks for your PR. Is there an easier way to reproduce this issue? That would be very nice. |
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
The problem with reproducing is that in Linux I cannot instantiate a WindowsPath, getting a NotImplementedError. I could write a test that generates a PureWindowsPath and modify the configuration_utils.py to check for instances of PureWindowsPath rather than WindowsPath. I am looking at tests/others/test_config.py to see if I can shoe horn that in somehow. |
|
Yeah that should work. Even a minimal reproducer that fails on your Windows environment should be good enough for us. |
…ath (depending on system) both work when converted to JSON
|
Added the unit test to tests/others/test_config.py |
…o_json_string(). Conditional now tests for Path, and uses Path.as_posix() to convert to string.
src/diffusers/configuration_utils.py
Outdated
| elif isinstance(value, PosixPath): | ||
| value = str(value) | ||
| elif isinstance(value, WindowsPath): | ||
| value = str(value.as_posix()) |
There was a problem hiding this comment.
I think the proper fix here would be to check if the value is a Path. Actually, PosixPath and WindowsPath are the 2 only subclasses of Path (see docs) and distinguishing both cases is not needed.
So what I would do is:
elif isinstance(value, Path):
value = value.as_posix()
as_posix is defined for any Path and directly returns a str.
Wauplin
left a comment
Wauplin
left a comment
There was a problem hiding this comment.
Looks good! Thanks for the changes @vincedovy 🤗
…ffusers into fix-json-windows-path-crash
|
@vincedovy we need to fix the code quality issues. https://github.com/huggingface/diffusers/actions/runs/9683518936/job/26719146234?pr=8662#step:6:1 provides guidance on how to fix it. |
|
Done |
* Add check for WindowsPath in to_json_string On Windows, os.path.join returns a WindowsPath. to_json_string does not convert this from a WindowsPath to a string. Added check for WindowsPath to to_json_saveable. * Remove extraneous convert to string in test_check_path_types (tests/others/test_config.py) * Fix style issues in tests/others/test_config.py * Add unit test to test_config.py to verify that PosixPath and WindowsPath (depending on system) both work when converted to JSON * Remove distinction between PosixPath and WindowsPath in ConfigMixIn.to_json_string(). Conditional now tests for Path, and uses Path.as_posix() to convert to string. --------- Co-authored-by: Vincent Dovydaitis <vincedovy@gmail.com> Co-authored-by: Sayak Paul <spsayakpaul@gmail.com>
4 participants
Running the diffusion-models-class/unit2/finetune_model.py line 117 (save_pretrained) and 01_finetuning_and_guidance.ipynb crashes trying to convert a WindowsPath to JSON. This worked prior to an conda update (not sure which module update caused the problem.) I have diffusers 0.29.0 installed. Issue was in configuration_utils.py line 590, where it checks whether the value is a PosixPath. In this case, it was a WindowsPath. Fix was to check for WindowsPath, convert to posix, and return the resulting string.
Fixes #8651
Before submitting
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
@sayakpaul @yiyixuxu @DN6
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.