Deprecate models script #30184
Deprecate models script #30184
Conversation
|
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. |
amyeroberts
force-pushed
the
deprecate-models-2
branch
from
f52900c
to
7066a3a
Compare
|
Nice. A tiny question: what the following means?
🙏 |
You're right - this isn't very clear. Essentially, all the actual imports e.g. like these ones in the modeling init, we can just do a string replace |
|
will try to look this on Monday/Tuesday. |
| for line in lines: | ||
| if line.startswith("# "): | ||
| new_model_lines.append(line) | ||
| new_model_lines.extend(tip_message_lines) |
There was a problem hiding this comment.
nit: just make sure we have the desired new lines. (could be checked by running the script - once it's ready to be merged)
There was a problem hiding this comment.
Sorry, I don't understand. Is the request to check that the file is modified as expected when the script is run?
I've done some test runs with different files. This is the diff I get on the model page for e.g. bert
diff --git a/docs/source/en/model_doc/bert.md b/docs/source/en/model_doc/bert.md
index c77a1d8525..c5115b1950 100644
--- a/docs/source/en/model_doc/bert.md
+++ b/docs/source/en/model_doc/bert.md
@@ -16,6 +16,14 @@ rendered properly in your Markdown viewer.
# BERT
+ <Tip warning={true}>
+
+ This model is in maintenance mode only, we don't accept any new PRs changing its code.
+ If you run into any issues running this model, please reinstall the last version that supported this model: v4.40.2.
+ You can do so by running the following command: `pip install -U transformers==4.40.2`.
+
+ </Tip>
+
<div class="flex flex-wrap space-x-1">
<a href="https://huggingface.co/models?filter=bert">
<img alt="Models" src="https://img.shields.io/badge/All_model_pages-bert-blueviolet">There was a problem hiding this comment.
No, my comment is just about the new lines we want to have around the new block.
From the provided sample, it looks correct 👍
| def move_model_files_to_deprecated(model): | ||
| model_path = REPO_PATH / f"src/transformers/models/{model}" | ||
| deprecated_model_path = REPO_PATH / f"src/transformers/models/deprecated/{model}" | ||
|
|
||
| if not os.path.exists(deprecated_model_path): | ||
| os.makedirs(deprecated_model_path) | ||
|
|
||
| for file in os.listdir(model_path): | ||
| if file == "__pycache__": | ||
| continue | ||
| repo.git.mv(f"{model_path}/{file}", f"{deprecated_model_path}/{file}") |
There was a problem hiding this comment.
It's not only moving the files. The import path should be updated too, see
There was a problem hiding this comment.
Just to double check I've understood - are you referring to the relative imports? I've updated now so that:
from .foo import bar -> from ..foo import bar
in the deprecated files
There was a problem hiding this comment.
Yes, the relative import paths. Great!
utils/deprecate_models.py
Outdated
| for i, line in enumerate(filelines.split("\n")): | ||
| indent = get_line_indent(line) | ||
|
|
||
| # We first handle the base objects imports | ||
| if line.startswith("_import_structure = {"): | ||
| in_base_imports = True | ||
| new_init_file_lines.append(line) | ||
|
|
||
| # Next line is in the else block | ||
| elif line.startswith("else:"): | ||
| new_init_file_lines.append(line) | ||
| in_else_block = True | ||
|
|
||
| # Not in the else block or base imports block - just add the line directly | ||
| elif not (in_else_block or in_base_imports): | ||
| new_init_file_lines.append(line) | ||
|
|
||
| elif in_else_block or in_base_imports: | ||
| # previous line(s) were a blank line but within the else block | ||
| if indent and maybe_else_block: | ||
| else_block.append(maybe_else_block) | ||
| maybe_else_block = [] | ||
|
|
||
| # We might be outside of the block, or it might just be a blank line | ||
| if not indent and line == "": | ||
| # End any existing sub_block and add it to the else block | ||
| else_block.append(sub_block) | ||
| sub_block = [] | ||
| maybe_else_block.append(line) | ||
|
|
||
| # We've exited the else-block or the base objects imports | ||
| elif (not indent and line != "") or line.strip().startswith("}"): | ||
| if sub_block: | ||
| else_block.append(sub_block) | ||
| sub_block = [] | ||
|
|
||
| else_block = [ | ||
| [sub_block] if not isinstance(sub_block, list) else sub_block for sub_block in else_block | ||
| ] | ||
|
|
||
| # Sort the sub-blocks in the else block | ||
| else_block = [sorted(sub_block) for sub_block in else_block] | ||
|
|
||
| # Flatten the lists so they're all lines | ||
| else_block = [line for sub_block in else_block for line in sub_block] | ||
|
|
||
| # Add the else block to the file lines and reset it | ||
| new_init_file_lines.extend(else_block) | ||
| else_block = [] | ||
| in_else_block = in_base_imports = False | ||
|
|
||
| # If we were in a maybe block, we now know it wasn't part of the else block | ||
| maybe_else_block.append(line) | ||
| new_init_file_lines.extend(maybe_else_block) | ||
| maybe_else_block = [] | ||
|
|
||
| elif line.endswith(("[", "(")) and not indented_block: | ||
| # We're at the start of an indented block | ||
| indented_block.append(line) | ||
| open_indent_level = indent | ||
|
|
||
| elif indented_block: | ||
| if indent == open_indent_level and ( | ||
| line.strip().endswith(("]", ")")) or line.strip().startswith(("],", "),")) | ||
| ): | ||
| # We're at the end of the indented block. Add the block to the sub-block and reset it | ||
| indented_block.append(line) | ||
| sub_block.append("\n".join(indented_block)) | ||
| indented_block = [] | ||
| open_indent_level = -1 | ||
| else: | ||
| # We're still in the indented block | ||
| indented_block.append(line) | ||
|
|
||
| # We have a comment line - create a new sub-block so lines above the comment are grouped together when sorting | ||
| elif line.strip().startswith("#"): | ||
| if sub_block: | ||
| else_block.append(sub_block) | ||
| sub_block = [] | ||
|
|
||
| # When adding else blocks to the file lines, we sort each sub-block, but not between sub-blocks. | ||
| # Comment lines are added directly to make sure they stay between the sub-blocks they're associated with | ||
| # structure: [sub_block1, comment1, sub_block2, comment2, ...] | ||
| else_block.append(line) | ||
|
|
||
| else: | ||
| sub_block.append(line) | ||
|
|
||
| # Add the last sub-block if it exists | ||
| if else_block: | ||
| # Sort the sub-blocks in the else block | ||
| else_block = [ | ||
| [sorted(sub_block) if isinstance(sub_block, list) else sub_block for sub_block in sub_blocks] | ||
| for sub_blocks in else_block | ||
| ] | ||
| # Flatten the lists so they're all lines | ||
| else_block = [line for sub_block in else_block for line in sub_block] | ||
| new_init_file_lines.extend(else_block) | ||
|
|
||
| return "\n".join(new_init_file_lines) |
There was a problem hiding this comment.
Thank you for the effort. Got to say I haven't dive into this yet. But IIRC, the file
utils/custom_init_isort.py
could be used to this purpose. Would you mind if it is what you can reuse?
There was a problem hiding this comment.
🤦♀️ I didn't know this existed - it would have saved me a lot of strife! Thanks for highlighting - and definitely in the camp of killing your darlings. I've removed this logic and just used the existing functionality
|
@ydshieh Thanks for your review! I've addressed or replied to all the comments - for some there's still some outstanding qs from my side. Should be ready for another review. |
|
@ydshieh Good point about making sure to still be able to load the models - it caught a bug! I've pushed a fix and confirmed that after that all the diffs I see when running the script are as expected and I can still load and run the model. |
What does this PR do?
Adds script
deprecate_modelswhich when called will deprecate the models in the repo. This includes:src/transformers/models/model->src/transformers/models/deprecated/modelruffto be in alphabetical order