Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

at::from_blob documentation is broken #47462

Closed
elbaro opened this issue Nov 5, 2020 · 13 comments
Closed

at::from_blob documentation is broken #47462

elbaro opened this issue Nov 5, 2020 · 13 comments
Assignees
Labels
module: cpp Related to C++ API module: docs Related to our documentation, both in docs/ and docblocks triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Comments

@elbaro
Copy link

elbaro commented Nov 5, 2020

📚 Documentation

https://pytorch.org/cppdocs/api/function_namespaceat_1a42d0b0697c2ec3f50d30ee4d6805a372.html?highlight=from_blob

doxygenfunction: Unable to resolve multiple matches for function “at::from_blob” with ...

cc @yf225 @glaringlee @jlin27 @mruberry

@elbaro elbaro changed the title at::from_blob at::from_blob documentation is broken Nov 5, 2020
@glaringlee glaringlee added module: cpp Related to C++ API module: docs Related to our documentation, both in docs/ and docblocks triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels Nov 6, 2020
@mruberry
Copy link
Collaborator

mruberry commented Nov 6, 2020

Thanks for reporting this issue, @elbaro. We would accept a PR fixing this.

@samestep samestep self-assigned this Nov 6, 2020
@samestep
Copy link
Contributor

samestep commented Nov 9, 2020

This issue seems much broader than just at::from_blob: searching for "doxygen" on pytorch.org/cppdocs yields 780 pages, all of which seem to show the same error message. Is there an easy way to figure out when these messages were introduced? I'd try git bisect but I feel like that would take too long.

@glaringlee
Copy link
Contributor

@samestep
This is probably since the beginning when we added c++ documents using doxygen + breathe + sphinx, and mainly happened in at namespace. We use breathe to translate doxygen comments, it doesn't work well on multiple function matching if you provide the function name only when it is an overloaded function (template specialization doesn't work well also).
I noticed that the higher version breathe might do this better, but I haven't tested yet. One possible fix is to add .rst and provide more specific doxygenfunction:: for ambiguous functions, but this is not a trivial fix, need to research on how to get all the function signatures, and whether need a codegen, etc.

Most people use C++ apis doc for NN Module and Optimizers which doesn't have multi function match issue (we take care of the doxygen comments within the code already). We got some similar issues before, we normally re-direct user to the cpp/h file to look at comments there at the moment.

@samestep samestep removed their assignment Nov 9, 2020
@samestep
Copy link
Contributor

samestep commented Nov 9, 2020

@glaringlee Thanks for the clarification! That sounds reasonable, in which case I probably don't have the expertise at the moment to resolve the issue. One thing I'm curious about, though: @malfet found that in pytorch/cppdocs@6ed43dd, there were 579 pages with this error message (and the number has since increased to 780), whereas in its parent commit pytorch/cppdocs@e00f7e6, there were only 6 (one of which is at::from_blob). So while I'm guessing you're right about this particular function, the broader appearance of this error message seems to be more recent; perhaps caused by #41312?

@glaringlee
Copy link
Contributor

@glaringlee Thanks for the clarification! That sounds reasonable, in which case I probably don't have the expertise at the moment to resolve the issue. One thing I'm curious about, though: @malfet found that in pytorch/cppdocs@6ed43dd, there were 579 pages with this error message (and the number has since increased to 780), whereas in its parent commit pytorch/cppdocs@e00f7e6, there were only 6 (one of which is at::from_blob). So while I'm guessing you're right about this particular function, the broader appearance of this error message seems to be more recent; perhaps caused by #41312?

@samestep Ah, I see what you mean now. I was not aware of this. @ezyang, can you advise? thx

@ezyang
Copy link
Contributor

ezyang commented Nov 10, 2020

To unblock for the point release, let's just revert the exhale version update, I didn't actually need it for anything. But to resolve the situation fully we will need to figure out what changed in the upgrade and come up with a strategy to deal with it.

@malfet malfet self-assigned this Nov 10, 2020
@malfet
Copy link
Contributor

malfet commented Nov 10, 2020

Tentatively grabbing for myself, let me have a look at what is going on with the version update (I don't think we can easily revert now, some of the packages gone stale)

facebook-github-bot pushed a commit that referenced this issue Nov 12, 2020
Summary:
This PR replaces the current auto-generated commit messages like pytorch/pytorch.github.io@fb217ab (currently includes no information) and pytorch/cppdocs@7efd67e (currently includes only a timestamp, which is redundant since it's a Git commit) with more descriptive ones that specify the pytorch/pytorch commit they originated from. This information would be useful for debugging issues such as #47462.

GitHub will also [autolink](https://docs.github.com/en/free-pro-team@latest/github/writing-on-github/autolinked-references-and-urls#commit-shas) these new messages (similar to pytorch/ci-hud@bc25ae7), and so they will now also mostly follow Git commit message conventions by starting with a capital letter, using the imperative voice, and (at least in the autolink-rendered form on GitHub, although not in the raw text) staying under 50 characters.

**Question for reviewers:** Will my `export CIRCLE_SHA1="$CIRCLE_SHA1"` work here? Is it necessary?

Pull Request resolved: #47694

Reviewed By: walterddr

Differential Revision: D24868240

Pulled By: samestep

fbshipit-source-id: 4907341e7b57ed6818ab550dc1ec423f2c2450c1
tugsbayasgalan pushed a commit to tugsbayasgalan/pytorch that referenced this issue Nov 16, 2020
Summary:
This PR replaces the current auto-generated commit messages like pytorch/pytorch.github.io@fb217ab (currently includes no information) and pytorch/cppdocs@7efd67e (currently includes only a timestamp, which is redundant since it's a Git commit) with more descriptive ones that specify the pytorch/pytorch commit they originated from. This information would be useful for debugging issues such as pytorch#47462.

GitHub will also [autolink](https://docs.github.com/en/free-pro-team@latest/github/writing-on-github/autolinked-references-and-urls#commit-shas) these new messages (similar to pytorch/ci-hud@bc25ae7), and so they will now also mostly follow Git commit message conventions by starting with a capital letter, using the imperative voice, and (at least in the autolink-rendered form on GitHub, although not in the raw text) staying under 50 characters.

**Question for reviewers:** Will my `export CIRCLE_SHA1="$CIRCLE_SHA1"` work here? Is it necessary?

Pull Request resolved: pytorch#47694

Reviewed By: walterddr

Differential Revision: D24868240

Pulled By: samestep

fbshipit-source-id: 4907341e7b57ed6818ab550dc1ec423f2c2450c1
@mattip mattip self-assigned this Nov 19, 2020
@mattip
Copy link
Collaborator

mattip commented Nov 30, 2020

It seems breathe is pinned at 4.19.2, the last release was 4.24. Maybe updating breathe would help?

@mattip
Copy link
Collaborator

mattip commented Dec 7, 2020

Up to breathe 4.14 there were two "Unable to resolve multiple matches" warnings generated by breathe. In 4.15-4.16, there were 13 warnings. Versions 4.17-4.18.0 of breathe cause sphinx errors:

breathe/renderer/sphinxrenderer.py", line 1519, in visit_friendclass \
     assert ''.join(n.astext() for n in self.render(node.get_type())) == "friend class"

Version 4.19.0 has 576 warnings.

There are sphinx domain warnings about "Duplicate declarations" (that changed in a sphinx 3.3 to "Duplicate C++ declarations"). These went down from 1500 in pre-1.19-breathe to less than 10 in breathe v1.19.

Exhale (which is used to run doxygen to create the XML files, then to call breathe to parse those XML files into sphinx RST direcectives) also emits a number of long warnings about places it detects that generate the same sphinx directive for functions with different signatures. However it only finds 4 of these: c10::overflows, torch::autograd::to_output_type, torch::data::make_data_loader, and torch::python::bind_module.

It would seem we need a better parser in breathe and a way to distinguish between the overridden declarations in sphinx in order to better fix this.

@ezyang
Copy link
Contributor

ezyang commented Dec 7, 2020

RIP. Maybe we should fund some improvements to breathe :/

@mattip
Copy link
Collaborator

mattip commented Dec 7, 2020

I distilled the failure down to a test I could add to breathe in the issue linked above this comment, maybe the author will be able to suggest a fix.

@mattip
Copy link
Collaborator

mattip commented Dec 9, 2020

I added a fix to the breathe PR breathe-doc/breathe#605. Using that cleans up all but 14 of the "Unable to resolve ..." warnings. The ones left are

at::from_blob(void *, IntArrayRef, IntArrayRef, const std::function<void(void *)>&, \
                       const TensorOptions&, const c10::optional<Device>)
at::from_blob(void *, IntArrayRef, IntArrayRef, const TensorOptions&)
at::from_blob(void *, IntArrayRef, const TensorOptions&)
at::from_blob(void *, IntArrayRef, const std::function<void(void *)>&, const TensorOptions&)
torch::from_blob(void *, at::IntArrayRef, const at::TensorOptions&)
torch::from_blob(void *, at::IntArrayRef, const Deleter&, const at::TensorOptions&)
torch::from_blob(void *, at::IntArrayRef, at::IntArrayRef, const at:
torch::from_blob(void *, at::IntArrayRef, at::IntArrayRef, const Deleter&, const at:
torch::schema(const char *, c10::AliasAnalysisKind)
torch::schema(const char *)
torch::init(Func&&)
torch::python::bind_module(py::module, const char *)
torch::python::bind_module(py::module, const char *)
torch::detail::constructSchemaOrName(const char *)

14 is better than 576.

@vermeeren
Copy link

vermeeren commented Dec 15, 2020

@mattip and others, I've just released Breathe 4.25.0 which should solve the (majority of) the problem. Thanks again Matti for the initial patchset and tests!

Edit: breathe-doc/breathe#606 was the fix used in the end.

hwangdeyu pushed a commit to hwangdeyu/pytorch that referenced this issue Jan 6, 2021
Summary:
Fixes pytorch#47462, but not completely.

Update breathe to the latest version to get fixes for the "Unable to resolve..." issues. There are still some build errors, but much fewer than before.

Pull Request resolved: pytorch#49407

Reviewed By: izdeby

Differential Revision: D25562163

Pulled By: glaringlee

fbshipit-source-id: 91bfd9e9ac70723816309f489022d72853f5fdc5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: cpp Related to C++ API module: docs Related to our documentation, both in docs/ and docblocks triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants