Refac docstrings in training_utils.py #9724

RogerSinghChugh · 2024-10-20T13:47:18Z

What does this PR do?

Fixes #9567

Before submitting

This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
Did you read the contributor guideline?
Did you read our philosophy doc (important for complex PRs)?
Was this discussed/approved via a GitHub issue or the forum? Please add a link to it if that's the case.
Did you make sure to update the documentation with your changes? Here are the
documentation guidelines, and
here are tips on formatting docstrings.
Did you write any new necessary tests?

Who can review?

@a-r-r-o-w @stevhliu

stevhliu

LGTM, thanks! Can you run make style to automatically apply the correct style corrections to fix the failing CI test?

HuggingFaceDocBuilderDev · 2024-10-21T17:08:20Z

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.

RogerSinghChugh · 2024-10-22T02:06:56Z

LGTM, thanks! Can you run make style to automatically apply the correct style corrections to fix the failing CI test?

Hi @stevhliu, did like you asked. Will make sure to run the command beforehand in the future.

stevhliu · 2024-10-22T17:16:22Z

Hmm, can you also please try running make quality?

RogerSinghChugh · 2024-10-23T03:43:57Z

Hmm, can you also please try running make quality?

Hi @stevhliu I tried running make quality and saw that it found 19 errors, none of them were in the files I changed though, should I be making those changes and then pushing the code?

Just for reference here is one of the errors I got from after running the make command.

tests\models\test_modeling_common.py:479:20: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
477 |             return
478 |
479 |         assert all(type(proc) == AttnProcessor2_0 for proc in model.attn_processors.values())
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ E721
480 |         with torch.no_grad():
481 |             if self.forward_requires_fresh_args:

a-r-r-o-w · 2024-10-23T03:45:14Z

Could you upgrade/downgrade your ruff version to 0.1.5 and try again?

RogerSinghChugh · 2024-10-23T04:00:34Z

Could you upgrade/downgrade your ruff version to 0.1.5 and try again?

Hi @a-r-r-o-w , after downgrading ruff to the version you specified, I get the following error messages:

ruff check examples scripts src tests utils benchmarks setup.py --fix
ruff format examples scripts src tests utils benchmarks setup.py
1114 files left unchanged
doc-builder style src/diffusers docs/source --max_len 119
process_begin: CreateProcess(NULL, doc-builder style src/diffusers docs/source --max_len 119, ...) failed.
make (e=2): The system cannot find the file specified.
make: *** [Makefile:59: style] Error 2

Trying to fix that, do you have any insights about this issue? Also apologies in advance if this is not the place to discuss issues like this.

RogerSinghChugh · 2024-10-23T05:06:38Z

Could you upgrade/downgrade your ruff version to 0.1.5 and try again?

Hi @a-r-r-o-w , after downgrading ruff to the version you specified, I get the following error messages:
ruff check examples scripts src tests utils benchmarks setup.py --fix
ruff format examples scripts src tests utils benchmarks setup.py
1114 files left unchanged
doc-builder style src/diffusers docs/source --max_len 119
process_begin: CreateProcess(NULL, doc-builder style src/diffusers docs/source --max_len 119, ...) failed.
make (e=2): The system cannot find the file specified.
make: *** [Makefile:59: style] Error 2
Trying to fix that, do you have any insights about this issue? Also apologies in advance if this is not the place to discuss issues like this.

Hi @a-r-r-o-w @stevhliu
I fixed the issues I was encountering.

I ran the two make commands.

Output from make quality:

ruff check examples scripts src tests utils benchmarks setup.py
src\diffusers\loaders\lora_pipeline.py:345:1: W293 [*] Blank line contains whitespace
src\diffusers\loaders\lora_pipeline.py:609:1: W293 [*] Blank line contains whitespace
src\diffusers\loaders\lora_pipeline.py:877:1: W293 [*] Blank line contains whitespace
src\diffusers\loaders\lora_pipeline.py:1242:1: W293 [*] Blank line contains whitespace
src\diffusers\loaders\lora_pipeline.py:1321:1: W293 [*] Blank line contains whitespace
src\diffusers\loaders\lora_pipeline.py:1448:1: W293 [*] Blank line contains whitespace
src\diffusers\loaders\lora_pipeline.py:1847:1: W293 [*] Blank line contains whitespace
src\diffusers\loaders\lora_pipeline.py:1918:1: W293 [*] Blank line contains whitespace
src\diffusers\loaders\lora_pipeline.py:2050:1: W293 [*] Blank line contains whitespace
src\diffusers\loaders\lora_pipeline.py:2417:1: W293 [*] Blank line contains whitespace
src\diffusers\loaders\lora_pipeline.py:2702:1: W293 [*] Blank line contains whitespace
src\diffusers\loaders\lora_pipeline.py:2756:1: W293 [*] Blank line contains whitespace
Found 12 errors.
[*] 12 fixable with the `--fix` option.
make: *** [Makefile:43: quality] Error 1

Output (partial, actual output is too big) from make style:

PS <pathToDiffusers>\diffusers> make style
ruff check examples scripts src tests utils benchmarks setup.py --fix
Found 12 errors (12 fixed, 0 remaining).
ruff format examples scripts src tests utils benchmarks setup.py
360 files reformatted, 754 files left unchanged
doc-builder style src/diffusers docs/source --max_len 119
Overwriting content of src/diffusers\callbacks.py.
Overwriting content of src/diffusers\commands\fp16_safetensors.py.
.
.
.
Overwriting content of src/diffusers\video_processor.py.
Cleaned 360 files!
<intialPath>/chocolatey/lib/make/tools/install/bin/make.exe autogenerate_code
make[1]: Entering directory '<pathToDiffusers>/diffusers'
running deps_table_update
updating src/diffusers/dependency_versions_table.py
make[1]: Leaving directory '<pathToDiffusers>/diffusers'
C:/ProgramData/chocolatey/lib/make/tools/install/bin/make.exe extra_style_checks
make[1]: Entering directory '<pathToDiffusers>/diffusers'
py utils/custom_init_isort.py
py utils/check_doc_toc.py --fix_and_overwrite
make[1]: Leaving directory '<pathToDiffusers>/diffusers'

What should my next steps be, do I have to run the make style with the --fix like mentioned in the output of that command? Also is there any documentation (which I might have missed) which contains details on how to proceed after running the make commands. I saw some documentation in CONTRIBUTING.md, is there any more?

Also just want to mention I really appreciate all the help I'm getting, thank you.

stevhliu · 2024-10-23T16:01:16Z

It looks like you should run make quality --fix, run make style, and then push those changes.

I think the How to open a PR is the only doc we have about running these commands, but we're always happy to help with any issues beyond that 🤗

RogerSinghChugh · 2024-10-24T19:12:16Z

It looks like you should run make quality --fix, run make style, and then push those changes.

I think the How to open a PR is the only doc we have about running these commands, but we're always happy to help with any issues beyond that 🤗

Thanks a lot for your help @stevhliu , so as per How to open a PR I tried running make style first and this is the output I got(no errors here but it modified a lot of files):

ruff check examples scripts src tests utils benchmarks setup.py --fix
ruff format examples scripts src tests utils benchmarks setup.py
1124 files left unchanged
doc-builder style src/diffusers docs/source --max_len 119
Overwriting content of src/diffusers\callbacks.py.
Overwriting content of src/diffusers\commands\fp16_safetensors.py.
.
.
Overwriting content of src/diffusers\video_processor.py.
Cleaned 365 files!
C:/ProgramData/chocolatey/lib/make/tools/install/bin/make.exe autogenerate_code
make[1]: Entering directory '<pathToDiffusers>/diffusers'
running deps_table_update
updating src/diffusers/dependency_versions_table.py
make[1]: Leaving directory '<pathToDiffusers>/diffusers'
C:/ProgramData/chocolatey/lib/make/tools/install/bin/make.exe extra_style_checks
make[1]: Entering directory '<pathToDiffusers>/diffusers'
python utils/custom_init_isort.py
python utils/check_doc_toc.py --fix_and_overwrite
make[1]: Leaving directory '<pathToDiffusers>/diffusers'

Then I ran make quality, got a similar response as last time:

ruff check examples scripts src tests utils benchmarks setup.py
src\diffusers\loaders\lora_pipeline.py:345:1: W293 [*] Blank line contains whitespace
src\diffusers\loaders\lora_pipeline.py:606:1: W293 [*] Blank line contains whitespace
src\diffusers\loaders\lora_pipeline.py:871:1: W293 [*] Blank line contains whitespace
src\diffusers\loaders\lora_pipeline.py:1233:1: W293 [*] Blank line contains whitespace
src\diffusers\loaders\lora_pipeline.py:1309:1: W293 [*] Blank line contains whitespace
src\diffusers\loaders\lora_pipeline.py:1433:1: W293 [*] Blank line contains whitespace
src\diffusers\loaders\lora_pipeline.py:1829:1: W293 [*] Blank line contains whitespace
src\diffusers\loaders\lora_pipeline.py:1897:1: W293 [*] Blank line contains whitespace
src\diffusers\loaders\lora_pipeline.py:2026:1: W293 [*] Blank line contains whitespace
src\diffusers\loaders\lora_pipeline.py:2390:1: W293 [*] Blank line contains whitespace
src\diffusers\loaders\lora_pipeline.py:2672:1: W293 [*] Blank line contains whitespace
src\diffusers\loaders\lora_pipeline.py:2723:1: W293 [*] Blank line contains whitespace
Found 12 errors.
[*] 12 fixable with the `--fix` option.
make: *** [Makefile:43: quality] Error 1

As per your suggestion I tried running make quality --fix but that didn't work, here is the output:

C:\ProgramData\chocolatey\lib\make\tools\install\bin\make.exe: unrecognized option '--fix'
Usage: make [options] [target] ...
Options:
  -b, -m                      Ignored for compatibility.
  -B, --always-make           Unconditionally make all targets.
.
.

I then figured out that the --fix option is to be used with ruff check $(check_dirs) setup.py that is part of the quality section of the makefile, tried doing that but then got this:

ruff check examples scripts src tests utils benchmarks setup.py --fix
Found 12 errors (12 fixed, 0 remaining).
ruff format --check examples scripts src tests utils benchmarks setup.py
Would reformat: src\diffusers\callbacks.py
Would reformat: src\diffusers\commands\fp16_safetensors.py
Would reformat: src\diffusers\configuration_utils.py
.
.
Would reformat: src\diffusers\video_processor.py
365 files would be reformatted, 759 files left unchanged
make: *** [Makefile:44: quality] Error 1

So I have met with another error here, where do I go from here? Is there anything else I can provide you with that'll help you in helping me? I also had to install hf-doc-builder to use the make file, if versioning of the package is an issue(like it was for ruff) please let me know, right now I'm running hf-doc-builder 0.5.0.

stevhliu · 2024-10-25T16:02:29Z

That is so strange, I'm not sure what the issue is. make style should've addressed the whitespace issues you're getting. Can you try rebasing your branch on main?

RogerSinghChugh · 2024-10-30T13:16:24Z

make quality

Hi @stevhliu.

I tried rebasing my branch with main, and then ran the commands again.
Same result as before :(
How should I proceed? Should I be making a new fork and start from scratch ?

stevhliu · 2024-10-30T16:14:44Z

Ah sorry I couldn't help fix these issues and thanks for your patience. Yeah if you don't mind, let's get a clean start and open a new PR?

RogerSinghChugh · 2024-10-30T19:15:45Z

Ah sorry I couldn't help fix these issues and thanks for your patience. Yeah if you don't mind, let's get a clean start and open a new PR?

No worries, this seems like a weird little bug :(
So I'll start a new PR, my workflow will be:
create a new fork --> make my changes again --> run make style --> run make quality --> raise a new PR like I did before --> delete the existing fork(will not do this immediately).

How should I deal with this PR ? Should I be deleting this right now, or do we wait till the new PR gets merged(hopefully)?

RogerSinghChugh · 2024-10-31T06:36:52Z

Ah sorry I couldn't help fix these issues and thanks for your patience. Yeah if you don't mind, let's get a clean start and open a new PR?

No worries, this seems like a weird little bug :( So I'll start a new PR, my workflow will be: create a new fork --> make my changes again --> run make style --> run make quality --> raise a new PR like I did before --> delete the existing fork(will not do this immediately).

How should I deal with this PR ? Should I be deleting this right now, or do we wait till the new PR gets merged(hopefully)?

So slight change to the workflow, had to first delete my fork and then create a new fork. @stevhliu

RogerSinghChugh added 2 commits October 20, 2024 19:00

Additions to docstrings in training_utils.py

0b9f8cc

Additions to docstrings in training_utils.py continued.

4a1c7d4

stevhliu approved these changes Oct 21, 2024

View reviewed changes

Changes after running make style.

220e27f

Merge branch 'main' into refac_training_utils

5ef13c7

Merge branch 'huggingface:main' into refac_training_utils

7b52b80

RogerSinghChugh closed this by deleting the head repository Oct 30, 2024

stevhliu mentioned this pull request Nov 1, 2024

Refac training utils.py #9815

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refac docstrings in training_utils.py #9724

Refac docstrings in training_utils.py #9724

RogerSinghChugh commented Oct 20, 2024

stevhliu left a comment

HuggingFaceDocBuilderDev commented Oct 21, 2024

RogerSinghChugh commented Oct 22, 2024

stevhliu commented Oct 22, 2024

RogerSinghChugh commented Oct 23, 2024

a-r-r-o-w commented Oct 23, 2024

RogerSinghChugh commented Oct 23, 2024 •

edited

Loading

RogerSinghChugh commented Oct 23, 2024

stevhliu commented Oct 23, 2024

RogerSinghChugh commented Oct 24, 2024

stevhliu commented Oct 25, 2024

RogerSinghChugh commented Oct 30, 2024 •

edited

Loading

stevhliu commented Oct 30, 2024

RogerSinghChugh commented Oct 30, 2024

RogerSinghChugh commented Oct 31, 2024 •

edited

Loading

Refac docstrings in training_utils.py #9724

Refac docstrings in training_utils.py #9724

Conversation

RogerSinghChugh commented Oct 20, 2024

What does this PR do?

Before submitting

Who can review?

stevhliu left a comment

Choose a reason for hiding this comment

HuggingFaceDocBuilderDev commented Oct 21, 2024

RogerSinghChugh commented Oct 22, 2024

stevhliu commented Oct 22, 2024

RogerSinghChugh commented Oct 23, 2024

a-r-r-o-w commented Oct 23, 2024

RogerSinghChugh commented Oct 23, 2024 • edited Loading

RogerSinghChugh commented Oct 23, 2024

stevhliu commented Oct 23, 2024

RogerSinghChugh commented Oct 24, 2024

stevhliu commented Oct 25, 2024

RogerSinghChugh commented Oct 30, 2024 • edited Loading

stevhliu commented Oct 30, 2024

RogerSinghChugh commented Oct 30, 2024

RogerSinghChugh commented Oct 31, 2024 • edited Loading

RogerSinghChugh commented Oct 23, 2024 •

edited

Loading

RogerSinghChugh commented Oct 30, 2024 •

edited

Loading

RogerSinghChugh commented Oct 31, 2024 •

edited

Loading