Repo checks: check documented methods exist #32320
Conversation
| @@ -413,22 +413,15 @@ def get_model_modules() -> List[str]: | |||
| "modeling_auto", | |||
| "modeling_encoder_decoder", | |||
| "modeling_marian", | |||
| "modeling_mmbt", | |||
| "modeling_outputs", | |||
There was a problem hiding this comment.
we've changed the model structure since 4 years ago, when this list was first created: transformers.models exclusively contains folders with models.
As such, we don't need to manually ignore these files.
| @@ -442,8 +435,7 @@ def get_model_modules() -> List[str]: | |||
| for submodule in dir(model_module): | |||
| if submodule.startswith("modeling") and submodule not in _ignore_modules: | |||
| modeling_module = getattr(model_module, submodule) | |||
| if inspect.ismodule(modeling_module): | |||
There was a problem hiding this comment.
redundant check in our current folder structure
| for doc_file in Path(PATH_TO_DOC).glob("**/*.rst"): | ||
| with open(doc_file, "r", encoding="utf-8", newline="\n") as f: | ||
| content = f.read() | ||
| raw_doc_objs = re.findall(r"(?:autoclass|autofunction):: transformers.(\S+)\s+", content) | ||
| documented_obj += [obj.split(".")[-1] for obj in raw_doc_objs] |
There was a problem hiding this comment.
We don't have rst doc files for quite a long time
| @@ -1094,50 +1126,6 @@ def check_model_type_doc_match(): | |||
| ) | |||
|
|
|||
|
|
|||
| # Re pattern to catch :obj:`xx`, :class:`xx`, :func:`xx` or :meth:`xx`. | |||
There was a problem hiding this comment.
We don't have rst doc files for quite a long time
| """Check all models are properly tested and documented.""" | ||
| print("Checking all models are included.") | ||
| """Check all models are tested and documented.""" | ||
| print(" - checking all models are included.") |
There was a problem hiding this comment.
nit: with the addition of spacing, we can clearly see that these prints belong to check_repo.py in make fixup, as opposed to a blob of text 🤗
| @@ -1054,7 +1053,7 @@ def ignore_undocumented(name: str) -> bool: | |||
|
|
|||
| def check_all_objects_are_documented(): | |||
| """Check all models are properly documented.""" | |||
| documented_objs = find_all_documented_objects() | |||
| documented_objs, documented_methods_map = find_all_documented_objects() | |||
There was a problem hiding this comment.
Note: I intend to use this new mapping in check_docstrings, to perform basic quality checks on the docstring of publicly documented methods
gante
force-pushed
the
check_documented_methods
branch
from
252a056
to
cd2e601
Compare
|
arf, this is getting docker image issues due to imports |
|
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. |
gante
force-pushed
the
check_documented_methods
branch
2 times, most recently
from
9230164
to
69ef594
Compare
gante
force-pushed
the
check_documented_methods
branch
2 times, most recently
from
a72b443
to
2846753
Compare
|
Cool tool @gante! LMK when you want me to take a look! |
gante
force-pushed
the
check_documented_methods
branch
from
2846753
to
eba80cf
Compare
|
@LysandreJik the PR is now ready 🙌 (it was failing before because the check repository CI image didn't have all needed dependencies to check methods in objects) |
What does this PR do?
Adds a check to confirm that publicly documented methods exist. For instance, commenting out a publicly documented method like
DynamicCache.to_legacy_cachetriggers this check, which in essence is a reminder to be extra careful with methods tagged as documented.In the process, also removed some old, unused logic.