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

Adding instructions on how to build docs #1779

Merged

Conversation

vincentpierre
Copy link
Contributor

And deleting code I do not understand

Motivation and Context

Adding some more documentation as it was not obvious to me how to build it.
I also had to remove some code to get it to work, so I am trying to see if that code is really needed.

How Has This Been Tested

Trying to run CI to see if the documentation generation still works.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have completed my CLA (see CONTRIBUTING)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

And deleting code I do not understand
@vincentpierre vincentpierre self-assigned this Jun 9, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Jun 9, 2022
docs/conf.py Outdated
@@ -7,13 +7,6 @@
import sys


# TODO make this less brittle
sys.path = [
os.path.join(os.path.dirname(__file__), "..", "src_python"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was useful so one doesn't need to remember to fiddle with PYTHONPATH when building the docs locally. (And I used the second commented-out line in my local build that has a slightly different layout of the build directory.)

Was it breaking something for you or why can't it stay?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was breaking something on my local docs building, I had to remove this line of code to make it work.
I just feels very hacky to use python to load a python path in order to properly import habitat_sim. If we need the user to modify their python path for habitat_sim to be able to import, then we should be clear about it.
What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The intent here was so I could do just

path/to/m.css/documentation/python.py habitat-sim/docs/conf.py

and it does whatever it needs to do to to build the documentation, in a self-contained way, without having to remember anything else. The config file does a lot of other patching of the imported modules to make it look nicer in the generated output and I don't feel that's hacky at all. If this would be done as part of the actual Habitat python code then sure, that would be bad, but not here I think.

What was the error you got here? Does the error not happen if you set up PYTHONPATH "the proper way"? I'm interested to learn what can go wrong when touching sys.path like this.

As an alternative, if this is really a no-go, could you put the PYTHONPATH setup to docs/build.sh and docs/build-public.sh instead, and remove it from the .circleci.yml file? That way it's self-contained somewhere else at least, and that way you wouldn't need to mention this in the README either, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted my changes and only left the changes to the README. I still think this is a hack and hiding these tricks is not going to help us on the long term, but if you think the ease of use it brings is worth it, I don't mind leaving it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks. If you come across a problem caused by this path patching again, please don't hesitate to bug me about it -- with this I'm responsible for any potential suffering anyone might have.

Copy link
Contributor

@aclegg3 aclegg3 left a comment

Choose a reason for hiding this comment

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

Looks good. You'll need to merge main for CI I think.

docs/README.md Outdated Show resolved Hide resolved
@vincentpierre vincentpierre merged commit 9fa1333 into facebookresearch:main Jun 15, 2022
@vincentpierre vincentpierre deleted the adding_docs_instructions branch June 15, 2022 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants