Sort init import #10801

sgugger · 2021-03-19T00:24:25Z

What does this PR do?

Not a high-priority item but I get bored at nights and I like writing those kinds of scripts 😅

So this PR adds a script to properly sort the import inside _import_structure because people have been absolutely ruthless and putting their objects in any kind of random order. That's not very feng-shui so I'm bringing back harmony by having the same sort as isort applied to all __init__ that contain an _import_structure.

sgugger · 2021-03-19T00:24:57Z

src/transformers/__init__.py

@@ -2170,7 +2166,7 @@
            TFLxmertPreTrainedModel,
            TFLxmertVisualFeatureEncoder,
        )
-        from .models.marian import TFMarian, TFMarianMTModel
+        from .models.marian import TFMarianModel, TFMarianMTModel


Completely unrelated to this PR, renamed that object to its proper name.

Nice catch!

LysandreJik

Very nice! LGTM

LysandreJik · 2021-03-19T01:57:45Z

src/transformers/__init__.py

@@ -2170,7 +2166,7 @@
            TFLxmertPreTrainedModel,
            TFLxmertVisualFeatureEncoder,
        )
-        from .models.marian import TFMarian, TFMarianMTModel
+        from .models.marian import TFMarianModel, TFMarianMTModel


Nice catch!

LysandreJik · 2021-03-19T02:00:11Z

Makefile

-	python utils/style_doc.py src/transformers docs/source --max_len 119
+	python utils/style_doc.py src/transformers docs/source --max_len 119 --check_only


If I'm not mistaken, this prevents your style_doc.py to do changes when used with make fixup, is that voluntary?

LysandreJik · 2021-03-19T02:00:41Z

Makefile

 	python utils/class_mapping_update.py

 # this target runs checks on all files
 quality:
 	black --check $(check_dirs)
 	isort --check-only $(check_dirs)
+	python utils/custom_init_isort.py --check_only


I would put this in the extra_quality_checks too so that it runs with make fixup

utils/custom_init_isort.py

Co-authored-by: Lysandre Debut <lysandre@huggingface.co>

sgugger · 2021-03-19T13:55:49Z

To address your comments on the MakeFile, I have removed some checks from extra_quality_checks because they are checks that modify content and make quality is only supposed to check, not change.

To have make fixup still work as intended, I put the checks that change content in extra_style_checks that is called both by make fixup and make style. Could you double-check it looks okay @LysandreJik and @stas00 ? Thanks!

LysandreJik

Your changes LGTM. I agree with you that @stas00's review would be welcome before we merge.

stas00

LGTM

Added another small tweak suggestion

stas00 · 2021-03-19T16:37:14Z

Makefile

 	python utils/check_copies.py
 	python utils/check_table.py
 	python utils/check_dummies.py
 	python utils/check_repo.py
-	python utils/style_doc.py src/transformers docs/source --max_len 119
 	python utils/class_mapping_update.py


should we create a new target for all the auto-generation items so it's loud-n-clear? It'd be something like:

autogenerate_code: deps_table_update python utils/class_mapping_update.py

as these aren't style or quality

Oh I hadn't realized class_mapping_update was writing things. It should also disappear from make quality then to go with the others in extra_style_checks.

For me:

make fixup -> applies the styling patches and checks everything passes quickly
make style -> applying styling patches and other auto-generated code
make quality -> only checks, does not write anything

I only ever use make fixup - the initial design was that fixup = style+quality on only modified files.

So it was created before we added 2 items that generated code, so if others prefer the quick version then fixup should include the autogenerate target too.

Actually, is there is a situation where fixup won't do the right thing already? When does one need to run the full make style?

class_mapping_update doesn't do anything if the relevant file hasn't changed so it's blazingly fast

we can do the same for deps_table_update so it'd be instant 99% of the time, as currently it always runs.

Yes fixup is style+quality on only modified files. My problem is that some processes that modify files have been added to extra-quality-checks which is run by quality on top of fixup. So fixup runs extra-quality-checks as well as extra-styling-checks to do both, I don't want to change that part.

See latest version for my proposal:

extra-quality-checks common processes for quality and fixup, checks that are fast or need to run on all files, not changing any file

extra-style-checks common processes for style and fixup, checks that are fast or need to run on all files, changing files if necessary

style style all files + other checks that are "writing" the right thing

quality check all files + other checks (in check only mode)

fixup same as style + quality but only on modified files.

Makefile

* Initial script * Add script to properly sort imports in init. * Add to the CI * Update utils/custom_init_isort.py Co-authored-by: Lysandre Debut <lysandre@huggingface.co> * Separate scripts that change content from quality * Move class_mapping_update to style_checks Co-authored-by: Lysandre Debut <lysandre@huggingface.co>

sgugger added 3 commits March 18, 2021 20:09

Initial script

16f9479

Add script to properly sort imports in init.

ce520fa

Add to the CI

ae154f6

sgugger requested a review from LysandreJik March 19, 2021 00:24

sgugger commented Mar 19, 2021

View reviewed changes

LysandreJik approved these changes Mar 19, 2021

View reviewed changes

sgugger and others added 2 commits March 19, 2021 09:52

Update utils/custom_init_isort.py

3e78efb

Co-authored-by: Lysandre Debut <lysandre@huggingface.co>

Separate scripts that change content from quality

ff01e89

LysandreJik approved these changes Mar 19, 2021

View reviewed changes

stas00 approved these changes Mar 19, 2021

View reviewed changes

Move class_mapping_update to style_checks

66ba6b0

stas00 reviewed Mar 19, 2021

View reviewed changes

Makefile Show resolved Hide resolved

sgugger merged commit 21e86f9 into master Mar 19, 2021

sgugger deleted the sort_init_import branch March 19, 2021 20:17

stas00 mentioned this pull request Mar 19, 2021

[makefile] autogenerate target #10814

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sort init import #10801

Sort init import #10801

sgugger commented Mar 19, 2021

sgugger Mar 19, 2021

LysandreJik Mar 19, 2021

LysandreJik left a comment

LysandreJik Mar 19, 2021

LysandreJik Mar 19, 2021

LysandreJik Mar 19, 2021

sgugger commented Mar 19, 2021

LysandreJik left a comment

stas00 left a comment

stas00 Mar 19, 2021 •

edited

Loading

sgugger Mar 19, 2021

stas00 Mar 19, 2021

stas00 Mar 19, 2021 •

edited

Loading

sgugger Mar 19, 2021 •

edited

Loading

sgugger Mar 19, 2021

		python utils/style_doc.py src/transformers docs/source --max_len 119
		python utils/style_doc.py src/transformers docs/source --max_len 119 --check_only

Sort init import #10801

Sort init import #10801

Conversation

sgugger commented Mar 19, 2021

What does this PR do?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LysandreJik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgugger commented Mar 19, 2021

LysandreJik left a comment

Choose a reason for hiding this comment

stas00 left a comment

Choose a reason for hiding this comment

stas00 Mar 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stas00 Mar 19, 2021 • edited Loading

Choose a reason for hiding this comment

sgugger Mar 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stas00 Mar 19, 2021 •

edited

Loading

stas00 Mar 19, 2021 •

edited

Loading

sgugger Mar 19, 2021 •

edited

Loading