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

[Tracker] change to posix for better Windows support #6585

Closed
sayakpaul opened this issue Jan 16, 2024 · 17 comments · Fixed by #6719, #6737 or #6781
Closed

[Tracker] change to posix for better Windows support #6585

sayakpaul opened this issue Jan 16, 2024 · 17 comments · Fixed by #6719, #6737 or #6781

Comments

@sayakpaul
Copy link
Member

In #6564, @fabiorigano introduced the use of Posix to better support Windows compatibility.

It'd be nice to change the instances of os.path.join() to Path(...).as_posix().

Feel free to open PRs for this and tag me.

While opening PRs, please target only ONE script at a time.

Let's go 🚀

@Bhavay-2001
Copy link
Contributor

Hi @sayakpaul, I checked the mentioned PR and also understood the issue. This is a documentation change right? We need to change every instance of os.path.join() to Path(...).as_posix()?

@charchit7
Copy link
Contributor

Hi @sayakpaul, I checked the mentioned PR and also understood the issue. This is a documentation change right? We need to change every instance of os.path.join() to Path(...).as_posix()?

Hey @Bhavay-2001 this is not a documentation change. Try going through this issue #6561 you'll understand why we are doing this.

@Bhavay-2001
Copy link
Contributor

Hi @charchit7, yes I understood that this is to provide windows support for the adapter.py. I previously thought that it was a documentation change issue, however I have changed the file and opened the corresponding PR.
Thanks

@gzguevara
Copy link
Contributor

gzguevara commented Jan 26, 2024

Hi, @sayakpaul! Does this issue request to change all os.path.join() instances in the whole Diffusers repo? Can I just take any script containing os.path.join() and change it?

@sayakpaul
Copy link
Member Author

I think so.

@gzguevara gzguevara mentioned this issue Jan 26, 2024
6 tasks
@gzguevara
Copy link
Contributor

@sayakpaul there are quite a few such instances in training examples. I will tackle a few. ok?

@sayakpaul
Copy link
Member Author

I think those are fine to leave as is because most of the training doesn't take place on Windows.

@stephen-iezzi
Copy link
Contributor

@sayakpaul I searched across the entire diffusers repo and 111 files contain instance(s) of os.path.join however as you previously mentioned a lot of that is related to training. I then did a targeted search within diffusers/src/diffusers and this revealed a much smaller subset that contains 11 files and ~55 instances of os.path.join. One of these 11 files was addressed by Bhavay in this PR. Would it be worth me going after the other 10 files, most of which are within diffusers/src/diffusers/utils and diffusers/src/diffusers/pipelines?

@sayakpaul
Copy link
Member Author

Yeah that works for me.

@Bhavay-2001
Copy link
Contributor

Hi @sayakpaul, could you please review my PR.
Thanks

@sayakpaul
Copy link
Member Author

Where is it?

@Bhavay-2001
Copy link
Contributor

Please find the PR here

@Bhavay-2001
Copy link
Contributor

Also, @Stepheni12 if you don't mind can we distribute the following tasks?

@stephen-iezzi
Copy link
Contributor

Yes, @Bhavay-2001 that sounds good! There were 11 files in total and your PR already did one of them so we can just split the other 10 from this subset. You can take the top 5 files which are:

src/diffusers/utils/hub_utils.py
src/diffusers/utils/constants.py
src/diffusers/models/modeling_flax_utils.py
src/diffusers/configuration_utils.py
src/diffusers/utils/dynamic_modules_utils.py

and then I'll take the bottom half of the list which are:

src/diffusers/pipelines/onnx_utils.py
src/diffusers/pipelines/pipeline_flax_utils.py
src/diffusers/models/modeling_utils.py
src/diffusers/utils/testing_utils.py
src/diffusers/pipelines/pipeline_utils.py

@Bhavay-2001
Copy link
Contributor

Sure @Stepheni12, I will update the mentioned files

This was referenced Jan 30, 2024
@Bhavay-2001
Copy link
Contributor

Hi @sayakpaul , @yiyixuxu it would be great if you guys could review the PRs associated with the issue and merge it or let us know of any changes to be made. Thanks

@Bhavay-2001
Copy link
Contributor

Bhavay-2001 commented Feb 23, 2024

The PRs are mentioned here. @sayakpaul

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