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

Develop api documentation update #5325

Merged
merged 12 commits into from
May 19, 2021
Merged

Conversation

miguelalonsojr
Copy link
Collaborator

Proposed change(s)

Describe the changes made in this PR.

Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)

Types of change(s)

  • Bug fix
  • New feature
  • Code refactor
  • Breaking change
  • Documentation update
  • Other (please describe)

Checklist

  • Added tests that prove my fix is effective or that my feature works
  • Updated the changelog (if applicable)
  • Updated the documentation (if applicable)
  • Updated the migration guide (if applicable)

Other comments

Adding API documentation for the mlagents_envs package.

Added Python Low Level API Documentation in addition to How to Use document. Added link to API documentation in How to Use document.
@CLAassistant
Copy link

CLAassistant commented Apr 28, 2021

CLA assistant check
All committers have signed the CLA.

@chriselion chriselion requested review from vincentpierre and removed request for chriselion April 28, 2021 23:14
Copy link
Contributor

@vincentpierre vincentpierre left a comment

Choose a reason for hiding this comment

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

  • It seems to be just a markdownification of the docstrings of the mlagents_envs package. I do not see how this is useful.
  • I assume (hope) you used a tool to generate this documentation and I think it would be a lot more useful if this PR contained the code to generate this documentation. You could also have a pre-commit hook to detect changes on the mlagents_envs package and regenerate the markdown when needed. This would be much more future-proof!

docs/Python-API-Documentation.md Outdated Show resolved Hide resolved
@miguelalonsojr
Copy link
Collaborator Author

  • It seems to be just a markdownification of the docstrings of the mlagents_envs package. I do not see how this is useful.

Perhaps. But at least one user/customer does find it useful, and likely more do as well. Also, I can think of several reasons why this would be useful:

  1. SEO for when users are searching for API info on the ML Agents Python API.
  2. Removes a pain point for users/customers to have to generate these themselves.
  3. Can easily be setup to be hosted on something like readthedocs.io
  • I assume (hope) you used a tool to generate this documentation and I think it would be a lot more useful if this PR contained the code to generate this documentation.

Yes, of course this was automatically generated. I used pydoc-markdown to generate the api docs. I can include the yaml config file for it in the mlagents_envs package. Great idea. I'll add this in.

*You could also have a pre-commit hook to detect changes on the mlagents_envs package and regenerate the markdown when needed. This would be much more future-proof!

Great suggestion. I was originally going to do that, but I wasn't sure what the team's best practices are/were. I'll add this in.

Copy link
Contributor

@vincentpierre vincentpierre left a comment

Choose a reason for hiding this comment

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

Made this to fix some of the issues in the comments: #5369

modules:
- name: mlagents_envs
file_name: Python-API-Documentation.md
submodules:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to simplify these submodules? Which files are included and which are not.
I was to make sure that if the organization of the files change or if the content of the files change there will be minimal changes to this file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately, you need to specify which modules explicitly when using pydoc-markdown. We have two options with pydoc-markdown:

  1. Build all the docs for the entire package, or
  2. Specify which modules/sub-modules to build the docs for

This is as simple as it's going to get. If there's a major refactoring to the API, e.g. package architecture changes, module renaming, etc., we have no choice but to update these files. If the content of the modules changes, pydoc-markdown should pick that up automatically with no changes to this config.

- name: mlagents_envs
file_name: Python-API-Documentation.md
submodules:
- env_utils
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think env_utils, communicator, registry.binary_utils, registry.remote_registry_entry, registry.base_registry_entry and maybe more should not be a part of this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok. I'll remove.

@vincentpierre
Copy link
Contributor

Feel free to merge #5369 into this branch when the tests pass.

- name: Install dependencies
run: |
python -m pip install --upgrade pip
pip install pydoc-markdown
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be a fixed version in case the pydoc-markdown has a breaking update.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

vincentpierre and others added 2 commits May 18, 2021 08:31
* Some edits to the documentation

* fix precommit

* Update ml-agents-envs/mlagents_envs/base_env.py

* regenerating markdown
Copy link
Contributor

@vincentpierre vincentpierre left a comment

Choose a reason for hiding this comment

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

Have you considered adding the link to the new markdown on this page? https://github.com/Unity-Technologies/ml-agents/tree/main/docs#api-docs
I think it would give this new doc more visibility.
Feel free to merge with or without this suggestion.
Thank you for your work!

@miguelalonsojr miguelalonsojr merged commit 61be828 into main May 19, 2021
@delete-merged-branch delete-merged-branch bot deleted the develop-api-documentation-update branch May 19, 2021 12:47
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants