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

build and upload docs from circleci #3259

Merged
merged 2 commits into from
Jan 22, 2021
Merged

build and upload docs from circleci #3259

merged 2 commits into from
Jan 22, 2021

Conversation

mattip
Copy link
Contributor

@mattip mattip commented Jan 17, 2021

Adapted from pytorch/audio#1033 and subsequent fixes. The idea is to build docs on every PR. On nightly and on tag runs, the built docs will be pushed to the gh-pages branch.

Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

Looks like CI failure is related, see https://app.circleci.com/pipelines/github/pytorch/vision/5820/workflows/8009e653-7c94-42fd-ac93-49c0247becc2/jobs/373582 for example

.circleci/unittest/linux/scripts/install.sh
.circleci/unittest/linux/scripts/install.sh: line 10: ./conda/bin/conda: No such file or directory
.circleci/unittest/linux/scripts/install.sh: line 11: conda: command not found

Exited with code exit status 127
CircleCI received exit code 127

Could you have a look at fixing this?

.circleci/build_docs/commit_docs.sh Outdated Show resolved Hide resolved
@mattip
Copy link
Contributor Author

mattip commented Jan 20, 2021

Ready for another review. This now uploads the artifacts, you can see them here.

Numpy uses this github action to get an extra ci/circleci: build artifact CI run, if you click on the details link it takes you right to the index.html page. For instance see the CI run in this PR, scroll to the bottom and there is the run with the ci/circleci: build artifact label

@mattip
Copy link
Contributor Author

mattip commented Jan 20, 2021

... but looking at the 64 68 builds here, it might be harder to find the link on the CI run box than to click through to the circle-ci interface, click on the build_docs, click on the artifacts tab, and choose one.

Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

I think it's fine for now for us to have to click on the Artifacts tab in the CircleCI job to get to the rendered page.

@fmassa fmassa merged commit e04de77 into pytorch:master Jan 22, 2021
@mattip
Copy link
Contributor Author

mattip commented Jan 22, 2021

Thanks. There may be latent problems with the upload script, but it only triggers on "nightly" builds. I will try to remember to look over the next few days.

@datumbox
Copy link
Contributor

@mattip just a heads up, @NicolasHug recently sent us a PR #3268 that fixes some of the formatting issues on the docs. Unfortunately we don't have a lint in place to check for important issues like incorrectly names variables names, missing vars etc. Speaking with @fmassa about this, we wonder if that's something that can be added on the nightly doc_build jobs in sphinx?

@mattip
Copy link
Contributor Author

mattip commented Jan 22, 2021

Sphinx can check some of these things. Note how on the build docs step of the CI run at the end it prints "build succeeded, 37 warnings.". We can turn these into errors, making the build fail. After the basic warnings are cleared up, sphinx has a nitpicky mode for things like improper formatting that can also be turned into errors. I would advise using it in the build run and not the nightly run, so PRs that cause new warnings fail before merging.
Of course that is only practical once the current warnings are fixed.

See pytorch/pytorch#50356, pytorch/pytorch#41335 for similar work in the pytorch repo.

@datumbox
Copy link
Contributor

@mattip Thanks for the references.

Note how on the build docs step of the CI run at the end it prints "build succeeded, 37 warnings.".

Looking at the warnings, I noticed that the linter reports many less critical indentation issues but misses some important mistakes on the documentation. Ideally we would like to capture formatting issues like those listed at #3003 and misspelled/missing/incorrect method parameters like the bboxes on the example below:

vision/torchvision/utils.py

Lines 141 to 157 in 767b23e

def draw_bounding_boxes(
image: torch.Tensor,
boxes: torch.Tensor,
labels: Optional[List[str]] = None,
colors: Optional[List[Union[str, Tuple[int, int, int]]]] = None,
width: int = 1,
font: Optional[str] = None,
font_size: int = 10
) -> torch.Tensor:
"""
Draws bounding boxes on given image.
The values of the input image should be uint8 between 0 and 255.
Args:
image (Tensor): Tensor of shape (C x H x W)
bboxes (Tensor): Tensor of size (N, 4) containing bounding boxes in (xmin, ymin, xmax, ymax) format. Note that

Is the specific lint configurable to flag the above?

I would advise using it in the build run and not the nightly run, so PRs that cause new warnings fail before merging. Of course that is only practical once the current warnings are fixed. See pytorch/pytorch#50356, pytorch/pytorch#41335 for similar work in the pytorch repo

Good to see that PyTorch has already adopted a solution. It's definitely worth exploring using a similar approach. We can bring this for discussion with the team.

facebook-github-bot pushed a commit that referenced this pull request Feb 1, 2021
Summary: Co-authored-by: Francisco Massa <fvsmassa@gmail.com>

Reviewed By: datumbox

Differential Revision: D26156376

fbshipit-source-id: 6888a944ce36e6733089ef13ef6c3b429cea8b85
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants