Fix multiple Sphinx warnings & docstrings #985
Conversation
ProGamerGov
changed the title
ProGamerGov
changed the title
For docstring formatting consistency
|
I've come across some issues:
|
|
I've also noticed a major mistake in formatting code snippets in docstrings. The other Captum modules all use this incorrect format that doesn't show up on the website:
They should be using this format, which works on the website and Markdown:
I'm working on a regex solution to fix this issue Captum wide, but that'll probably be for another PR. You can also directly link to function / classes based on their name like I show below, but this is not something I can easily automate: |
Thank you for the suggestion, @ProGamerGov! Regarding any or Any currently I don't see that the website is linking to the right type. Basically it is currently not generating any link on the web site. I see the same issue for Callable. I think that we can use tuple of int - I think that we don't need to add 's' in the end I just also looked into pytorch docs and it looks like they just annotate it with dict Do you mean this for tuples: (type1 or type2), and (type1, type2, or type3) ? |
|
@NarineK So for Callable and Any, I am planning on using this function in the There aren't any links for Callable or Any currently, but the code below will ensure that they are created when generating the Sphinx webpages. (This function hasn't been added to this PR as I was planning to add it with the rest of the Optim module. However, I can add it to the master branch now if you want.)
I was referring to listing multiple types for a variable in the docs like this: I think I saw a few cases where this was used: |
* generator -> Generator
|
PyTorch now officially has the same docstring type guidelines as the ones that I've created in this PR for Captum, now that my PR has been merged! https://github.com/pytorch/pytorch/blob/master/CONTRIBUTING.md#docstring-type-formatting |
|
@ProGamerGov This PR has contained too many contents for me to directly review. Could you help me break down things a little bit. Based on my understanding, the changes can be categorized into the following areas:
Did I miss anything? |
|
@aobo-y So in addition to what you listed, there are the following changes:
I can easily change all references of lowercase |
|
Great! If the |
|
I just made the following changes, and now am working on changing the lowercase tensor to uppercase.
|
|
@aobo-y Okay, I just converted all docstring type instances of |
|
@aobo-y I just noticed a new warning in the libmamba test. I thought it might be causing the Conda test to fail, but it still seems to be working though there's an extra 10 minutes of install time now. I reported the issue to the Conda devs: conda/conda#11790 All tests have passed! |
| lines[i] = re.sub(_rt[0] + r"Any" + _rt[1], "~typing.Any", lines[i]) | ||
| lines[i] = re.sub(_rt[0] + r"Callable" + _rt[1], "~typing.Callable", lines[i]) | ||
| lines[i] = re.sub(_rt[0] + r"Iterator" + _rt[1], "~typing.Iterator", lines[i]) | ||
| lines[i] = re.sub(_rt[0] + r"Iterable" + _rt[1], "~typing.Iterable", lines[i]) |
There was a problem hiding this comment.
why do we need to add ~typing. for them? what is the exact situation that they cannot be identified?
There was a problem hiding this comment.
@aobo-y Adding ~typing. to them ensures that intersphinx recognizes them properly, as it does not autodetect anything under the typing module. This way it shows up as hyperlinked (the ~typing. portion doesn't show up int the HTML docs).
There was a problem hiding this comment.
hmm, then wondering if you have any clues why Tensor can be correctly linked with from torch import Tensor but not Iterable with from typing import Iterable?
There was a problem hiding this comment.
@aobo-y I'm not sure. Even inferring Tensor from the imports doesn't seem to be very robust as it only does it 3 times in total for the entire project.
These are the type hints before autodoc_process_docstring, with each line showing the name of the class / function and then the docstring type (name, lines[i]): https://gist.github.com/ProGamerGov/1d0636cd173f46effc8db7a2705f509d
And these are the type hints after: https://gist.github.com/ProGamerGov/31871f2d231ab68179ab18eecabb77b6
There was a problem hiding this comment.
I had thought that there were a few more cases where Sphinx detected the from torch import Tensor part, but these are the only ones:
captum.attr.LayerAttribution.interpolate :type layer_attribution: :py:class:`~torch.Tensor`
captum.influence.TracInCPFastRandProj.influence :type targets: :py:class:`~torch.Tensor`
captum.attr.LRP.compute_convergence_delta :type output: :py:class:`~torch.Tensor`
There are no cases where it detects the typing module imports. So I guess we can basically just assume that it doesn't detect any from torch import Tensor imports then.
Weirdly enough a small handful of random Python type usage instances get detected and wrapped in intersphinx supported code before the replacement function, like:py:class:'<str>'. Though somewhere down the line all of them get properly detected, which is a bit confusing.
There was a problem hiding this comment.
Interesting. TracInCPFast and TracInCPFastRandProj are under the same file with the same docstring, but one is detected and one is not. Indeed pretty weird
captum.influence.TracInCPFast.influence :type targets: Tensor, optional
...
captum.influence.TracInCPFastRandProj.influence :type targets: :py:class:`~torch.Tensor`
| # Preserve signature defaults | ||
| # Prevents entire tensors from being printed, & gives callable functions | ||
| # proper names | ||
| autodoc_preserve_defaults = True |
There was a problem hiding this comment.
Can you point me to where we use variables as default? I am thinking for some of these cases, we can set the default as None and assign the variable within the code after checking None
There was a problem hiding this comment.
@aobo-y Here's an example: https://captum.ai/api/influence.html#similarityinfluence
The similarity_metric variable is set to the cosine_similarity function, and it shows up in the docs with additional characters wrapped around the name:
similarity_metric=<function cosine_similarity>
Setting autodoc_preserve_defaults = True corrects it.
similarity_metric=cosine_similarity
This setting is also something I need for the Optim module's TransformationRobustness class:
# Define TransformationRobustness defaults externally for easier Sphinx docs formatting
_TR_TRANSLATE: List[int] = [4] * 10
_TR_SCALE: List[float] = [0.995**n for n in range(-5, 80)] + [
0.998**n for n in 2 * list(range(20, 40))
]
_TR_DEGREES: List[int] = (
list(range(-20, 20)) + list(range(-10, 10)) + list(range(-5, 5)) + 5 * [0]
)
class TransformationRobustness(nn.Module):
def __init__(
self,
padding_transform: Optional[nn.Module] = nn.ConstantPad2d(2, value=0.5),
translate: Optional[Union[int, List[int]]] = _TR_TRANSLATE,
scale: Optional[NumSeqOrTensorOrProbDistType] = _TR_SCALE,
degrees: Optional[NumSeqOrTensorOrProbDistType] = _TR_DEGREES,
final_translate: Optional[int] = 2,
crop_or_pad_output: bool = False,
) -> None:
This way I don't need a second variable to disable each option, and Sphinx doesn't try to show hundreds of numbers in the HTML docs.
There was a problem hiding this comment.
generally, we do not encourage using variables as default. But there are surely some cases that it may make more sense to use variables. Using huge list/tensor as default is something that does not exist before. We can discuss the optim example further when reviewing the optim module.
For existing code, I believe the only variable defaults are all functions, like the example you give. Then actually, <function cosine_similarity> is slightly more preferable, but no big difference, so we can keep this change.
Please correct me if you are aware of other cases in our codebase.
There was a problem hiding this comment.
@aobo-y I think that there may be a few other variables with function defaults cases, but I don't recall any other variables in the Captum at the moment that would be affected by this change.
|
I added the |
* list of list of Concept -> list[list[Concept]] * Return Types: tensor & tensors -> Tensor
|
@aobo-y has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
Hi @ProGamerGov , I cannot merge this pr coz there is a conflict in |
|
@aobo-y I already fixed the merge conflicts in a PR earlier today, but I just noticed a mypy issue present in the master branch that was causing lint errors. I've fixed that mypy issue with the latest commit |
|
@aobo-y has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
The warning messages take up a lot of space on the console log, and it was really simple to resolve them.
The common.rst file was also incorrectly pointing to the wrong path and some of functions were renamed since it was created, so no docs were being generated for that page.
InputBaselineXGradientwas also removed from the public rst api docs, as it's not supposed to be public.In addition to these easy doc warning fixes, I found a module that was listed on on the API docs site, but there's not public path to use it and no tests were ever written for it. I made an issue post for it here: #989
I fixed some docstring issues like lack of consistent uppercase for
anyandcallable, spacing, random type / case mistakes I came across, etc...I also fixed some paths.
Issues with upgrading to later versions of Sphinx were also resolved.
Currently Sphinx gives the following warnings / errors for the master branch:
With the changes in this PR, Sphinx now only gives the following warnings instead of the above multi page list: