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

Unified documentation for repo #993

Merged
merged 18 commits into from
Feb 19, 2022
Merged

Unified documentation for repo #993

merged 18 commits into from
Feb 19, 2022

Conversation

1heart
Copy link
Contributor

@1heart 1heart commented Feb 15, 2022

Description

Unifies documentation for all projects in the repo under facebookresearch.github.io/fairo.

Added droidlet autogenerated documentation in CircleCI.

Note:

I considered using a single Sphinx installation for all projects, but each project requires setting up its own environment in order to properly auto-generate docs from code comments. I think a more sustainable model is having each project generate its own docs and persist that in a CircleCI Workspace, following the model in this blogpost. All projects' docs get aggregated in the final CI job and published to gh-pages, with a single entrypoint index.html for Fairo itself.

Once we have a single conda environment containing all fairo projects, then we can move to a single Sphinx install; this is out of scope for this PR.

TODO:

  • Get ssh write access to repo for upload bot
  • Generate Droidlet docs in CircleCI
  • Improve root index.html -- currently very plain
  • Unified Sphinx theme?
  • Reapply Auto-push formatting fixes for Python #1000 after review (revert 81efbaa)
  • Reapply main branch filter after review

Future PR:

  • Once we have a unified conda environment building all jobs (droidlet, polymetis, fbrp, perception stack), we can actually have a single Sphinx doc website with all projects linking to each other on the sidebar.

  • Bug fix (non-breaking change that fixes an issue)
  • Proposes a change (non-breaking change that isn't necessarily a bug)
  • Refactor
  • New feature (non-breaking change that adds a new functionality)
  • Breaking change (fix or feature that would break some existing functionality downstream)
  • This is a unit test
  • Documentation only change
  • Datasets Release
  • Models Release

Type of requested review

  • I want a thorough review of the implementation.
  • I want a high level review.
  • I want a deep design review.

Before and After

Screen Shot 2022-02-14 at 11 11 36 PM

Testing

Copy generated docs into docs/project-name, then host a webserver serving this folder (e.g. python -m http.server).

Checklist:

  • I have performed manual end-to-end testing of the feature in my environment.
  • I have added Docstrings and comments to the code.
  • I have made changes to existing documentation where needed.
  • I have added tests that show that the PR is functional.
  • New and existing unit tests pass locally with my changes.
  • I have added relevant collaborators to review the PR before merge.
  • [Polymetis only] I ran on hardware (1) all scripts in tests/scripts, (2) asv benchmarks.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 15, 2022
1heart and others added 11 commits February 14, 2022 23:17
* Formatting fixes

* Add push key

* [skip ci] Automatic style fix for droidlet

* Use bot as id for pushes

* Depend on minecraft so workspace is updated with droidlet docs

* Main branch only

* Install awscli

Co-authored-by: Yuxuan Sun <yuxuans@fb.com>
@1heart 1heart marked this pull request as ready for review February 16, 2022 22:05
Copy link
Contributor

@exhaustin exhaustin 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 in general!

Cmiiw, the current workflow within .circleci/config.yml for adding a documentation of a new project looks like: build docs into a folder -> copy html files to doc directory /tmp/workspace/docs/project_name -> persisting that doc directory.
Is it possible to deduplicate these steps across different packages by maybe having a command to complete these actions automatically assuming the env has been set up and that docs are placed in fairo/project_name/docs?

To be clear, the config for a project would look something like:

jobs:
...
  project_name:
    ...
    steps:
    - checkout
    - run:
        name: Setup env & install project
        ...
    - run:
        name: Run tests
        ...
    - build_and_register_docs

Thus adding docs for a new project becomes a simple task of correctly placing sphinx source files and adding a single command to the CircleCI config.

@1heart 1heart force-pushed the yixinlin/unified-docs branch from 2a05b67 to b564f2a Compare February 18, 2022 22:39
@codecov-commenter
Copy link

Codecov Report

Merging #993 (ca5b634) into main (246ba18) will increase coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #993      +/-   ##
==========================================
+ Coverage   33.16%   33.23%   +0.06%     
==========================================
  Files         346      347       +1     
  Lines       34034    34279     +245     
==========================================
+ Hits        11286    11391     +105     
- Misses      22748    22888     +140     
Impacted Files Coverage Δ
agents/_agent.py 44.08% <0.00%> (-7.69%) ⬇️
interpreter/craftassist/tasks.py 74.45% <0.00%> (-0.55%) ⬇️
agents/droidlet_agent.py 51.76% <0.00%> (ø)

@exhaustin exhaustin self-requested a review February 19, 2022 00:02
Copy link
Contributor

@exhaustin exhaustin left a comment

Choose a reason for hiding this comment

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

LGTM, approving to unblock merging, but feel free to wait for other reviewers since this change affects multiple projects.
Thanks!

@1heart 1heart merged commit ea4d3df into main Feb 19, 2022
@1heart 1heart deleted the yixinlin/unified-docs branch February 19, 2022 00:22
@1heart 1heart mentioned this pull request Feb 24, 2022
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants