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

feat: add automatic RST generator for internal API docs #293

Closed

Conversation

imprvhub
Copy link

@imprvhub imprvhub commented Dec 11, 2024

This PR adds functionality to automatically generate .rst files for internal API documentation.

Changes:

  • Add generate_internal_api_rst.py script that:
    • Automatically detects internal modules
    • Generates appropriate RST files with consistent formatting
    • Maintains hierarchical module structure
    • Provides meaningful module descriptions
  • Add tests for core functionality
  • Keep consistent docstring formats

The script generates RST files in docs/api/internal/ with proper module
hierarchy and descriptions, removing the need for manual maintenance.

Testing:

  1. Run the script:
python cicd_utils/cicd/scripts/generate_internal_api_rst.py
  1. Check generated files in docs/api/internal/

  2. Run tests:

tox -e pytest -- tests/cicd_utils/test_scripts/test_generate_internal_api_rst.py

Note: The script can be integrated into the CI pipeline in a future iteration if needed.

PR check list

  • Read the contributing guidelines.
    • You don't have to read the whole thing, but it's a good idea to skim it. And definitely take a look at it if you're experiencing any issues getting your local environment up and running.
  • Provided the relevant details in the PR's description.
  • Referenced relevant issues in the PR's description.
  • Added tests for all my changes.
    • The CI will fail unless 100% of all new code is covered by the tests.
  • Updated the docs for relevant changes.
    • New modules (or renamed ones) are included in docs/api/internal/.
    • New public functions/classes/variables are documented in docs/api/index.rst.
    • Added the appropriate versionadded, versionchanged, or deprecated directives to docstrings.
      • The version should be the next release version, which you can infer by bumping the minor version in MAJOR.MINOR.PATCH (e.g., if the current version is 0.2.3, the next release will be 0.3.0).
  • Changes are documented in docs/reference/changelog.md.
    • Please try to follow the conventions laid out in the file. In doubt, just ask!
  • Consider granting push permissions to your PR's branch, so maintainers can help you out if needed.
  • The CI check are all passing, or I'm working on fixing them!
  • I have reviewed my own code! 🤠

📚 Documentation preview 📚: https://ridgeplot--293.org.readthedocs.build/en/293/

Closes #290

This commit adds functionality to automatically generate .rst files
for internal API documentation, addressing tpvasconcelos#290.

Changes include:
- Add script to automatically generate RST files
- Support hierarchical module structure
- Maintain consistent module descriptions
- Add basic tests for core functionality

The script detects all internal modules and generates appropriate
.rst files with consistent formatting and structure.
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for submitting your first pull request with us! 🎉

Our response times may vary, but we'll get back to you as soon as we can!

To help us help you, please make sure you have ticked all the boxes in the pull request template.

Welcome aboard! 🚀

…nternal_api_rst.py

This PR fixes two issues:

Sets the correct executable permissions for the generate_internal_api_rst.py script to ensure it runs properly in the CI/CD pipeline.
Resolves issues in test_generate_internal_api_rst.py.
Changes:

Set executable permissions for generate_internal_api_rst.py.
Updated test_generate_internal_api_rst.py to resolve related issues.
Testing:

Ensure the script runs without permission errors in the CI/CD pipeline.
Verify that tests pass successfully.
@imprvhub imprvhub force-pushed the feat/auto-generate-internal-docs branch from 28b7e87 to 5188dba Compare December 11, 2024 08:06
Copy link
Owner

@tpvasconcelos tpvasconcelos left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution @imprvhub!
I haven't had the time to properly go through your solution yet but let me leave a few first-pass comments:

  • could you add this to the sphinx build step? We already have one or two custom steps (eg for regenerating the example plots) so you could just copy and adapt for the function you need to call

  • since these will not be automatically generated during the build step, you can remove them from git and add an entry to gitignore (eg docs/api/internal/)

  • instead of iterating through the python files and local directory tree, I would suggest iterating through the installed package using a standard utility like pkgutil. For instance:

    import ridgeplot
    import pkgutil
    
    for _, module_name, is_package in pkgutil.iter_modules(ridgeplot.__path__):
        # ...
    
  • I think I like your idea of having a simple dictionary with short descriptions, however I think I would prefer defaulting to adding these short descriptions as module docstrings and only using the dictionary for overriding this default behaviour (e.g. if the module's docstrings is too long for instance)

If some of the comments don't seem to make sense or you think that I somehow misread your implementation/intention here, don't worry too much about it because I'm looking at this on my phone so let me take a proper look at this in the coming days 😁

Again, thanks a lot for wanting to help! I really appreciate it 🙏🏽

@tpvasconcelos
Copy link
Owner

tpvasconcelos commented Dec 12, 2024

@imprvhub this is what I mean by adding this as a build hook in Sphinx:

https://github.com/tpvasconcelos/ridgeplot/blob/main/docs/conf.py#L394-L395

But you'll probably need to use a different target event name since this needs to happen before the build step I guess

Copy link
Owner

@tpvasconcelos tpvasconcelos left a comment

Choose a reason for hiding this comment

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

Btw some CI checks are failing.
If you find it easier to iterate and test things locally, take a look at: https://ridgeplot.readthedocs.io/en/latest/development/contributing.html#development-environment

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated no suggestions.

@tpvasconcelos
Copy link
Owner

tpvasconcelos commented Dec 12, 2024

Btw I added :octocat: Copilot as a "reviewer" because I literally just got access to this new feature and I'm interested in how well/not-so-well it works. So please feel free to completely ignore its comments if you want 😉

reference: https://docs.github.com/en/copilot/using-github-copilot/code-review/using-copilot-code-review?tool=webui#requesting-a-review-from-copilot

imprvhub and others added 3 commits December 12, 2024 09:15
- Added `.gitignore` entry to exclude `docs/api/internal/` from version control.
- Refactored `generate_internal_api_rst.py`:
  - Improved module scanning logic for internal API documentation.
  - Enhanced docstring parsing for better description generation.
- Added `test_generate_internal_api_rst.py` for coverage of:
  - Module organization.
  - RST content generation for internal modules.

Pending improvements:
- Integration with Sphinx build step (`conf.py`).
- Transition to `pkgutil` for module iteration.
- Adjusted file permissions for `tests/cicd_utils/test_scripts/test_generate_internal_api_rst.py` to remove executable flag.
- Applied `chmod -x` to correct invalid or unnecessary file attributes.

This change ensures compliance with standard file permission practices for test scripts.
@imprvhub
Copy link
Author

Hey @tpvasconcelos! 👋

Thanks for the detailed review and suggestions! I've implemented most of your feedback in this commit.

About the remaining suggestions:

  1. Regarding pkgutil - I tried implementing it but ran into some issues with nested modules and test coverage. The current implementation using direct file traversal seems more reliable for catching all internal modules, but I'm open to revisiting this if you have specific ideas!

  2. For the Sphinx build integration - I've started working on it but wanted to ensure it doesn't break any existing functionality. The tricky part is handling the event timing to ensure RST generation happens at the right moment in the build process.

Most tests are passing locally (they are quite thorough! 😅), but I wanted to get your thoughts before making more changes.

Thanks again for the opportunity to contribute! Let me know if you'd like me to adjust anything else.

Added entry to docs/reference/changelog.md to pass
peter-evans documentation check. Documents the automatic
RST file generation feature.
@tpvasconcelos tpvasconcelos self-assigned this Dec 13, 2024

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

@tpvasconcelos
Copy link
Owner

Hey @imprvhub ! Just tried to run this locally and the docs are currently not rendering properly for me. I think that this is due to two issues:

  1. missing underscore for private modules. e.g., rendering .. automodule:: ridgeplot.figure_factory instead of .. automodule:: ridgeplot._figure_factory
  2. The toctree for nested packages is probably not correctly defined. The current implementation does something like _package/* while you are manually defining each submodule by name but missing the _package/ prefix

Let me know if you'd like me to take a closer look or whether you want to try to fix this yourself first! 👍

Btw I pushed a couple of commits to remove the current API docs from git and add your function as a hook to automatically rebuild the pages every time the docs are built.

imprvhub and others added 3 commits December 16, 2024 14:14
- Add missing underscore prefix for private modules in `automodule` directives (e.g., `ridgeplot._figure_factory` instead of `ridgeplot.figure_factory`).
- Correct `toctree` definitions to include the `_package/` prefix for nested packages.
- Update internal API docs to preserve underscore prefixes in paths.
- Adjust `organize_modules` hierarchy tests to reflect updated path expectations.
- Adjusted file permissions for `tests/cicd_utils/test_scripts/test_generate_internal_api_rst.py` to remove executable flag.
- Applied `chmod -x` to correct invalid or unnecessary file attributes.

This change ensures compliance with standard file permission practices for test scripts.
@imprvhub
Copy link
Author

imprvhub commented Dec 16, 2024

Hi @tpvasconcelos,

Thanks for the feedback! I’ve made the changes you mentioned:

• Added the underscore prefix for private modules in the automodule directives.
• Fixed the toctree definitions to include the _package/ prefix for nested packages.
• Updated the internal API docs and adjusted the organize_modules hierarchy tests.

I’ve tested these changes locally, and it seems to be working well here--though, I can’t promise it’ll pass all the tests just yet. Anyway, it might be worth giving it a shot on your local environment to see if the .rst files are generated correctly according to your expectations. Hopefully, it’ll work better for you too! Let me know how it goes, and thanks again for your help! 👍

@tpvasconcelos
Copy link
Owner

Hey @imprvhub for some reason I still can't render the docs properly on my machine. Could you share a screenshot of what you see on your end?

I still see the private modules without the leading underscore:

image

and the inner packages do now show their children:

image

I also still see this in my terminal output:

image

Let me know if you'd like me to take a look at it 👍

@imprvhub
Copy link
Author

python cicd_utils/cicd/scripts/generate_internal_api_rst.py
Captura de pantalla 2024-12-18 a la(s) 12 23 11 Captura de pantalla 2024-12-18 a la(s) 12 26 18 Captura de pantalla 2024-12-18 a la(s) 12 27 03

@imprvhub
Copy link
Author

imprvhub commented Dec 18, 2024

@tpvasconcelos Let me clarify what I'm showing in the screenshots:

• In the first terminal output, you can see the list of all generated RST files with their proper names including underscores
• In the directory tree screenshot, I'm showing the subdirectories and files properly created with leading underscores in the structure
• In the final screenshot, you can see an example of how base.rst is rendered, which includes proper automodule directives

Perhaps I misunderstood how you'd like the RST files to be generated or structured. Could you point out anything specific that looks incorrect? I'm happy to work on adjustments if needed. 👍

@tpvasconcelos
Copy link
Owner

@imprvhub in the PR's description you will notice a link at the bottom that is automatically generated by our CI pipeline that renders the docs for your branch. If you navigate to https://ridgeplot--293.org.readthedocs.build/en/293/api/index.html you can see what I mean.

If you want to iterate locally to try to fix the issue, you can run the following in your terminal:

tox -e docs-static

If you have any issues getting this to run on your local development environment, please take a look at our contributing docs.

@tpvasconcelos
Copy link
Owner

@imprvhub take a look at a different approach using sphinxcontrib.apidoc here --> #296

I think that this approach is simpler and more reliable than what we are trying to do here. One thing missing from that approach is a nice way to render a similar toc-tree for the internal packages, but maybe we can think of a different solution/approach.

I'll sleep on this and try to see if I can think of a better solution. Feel free to take a look at it in the meantime :)

@imprvhub
Copy link
Author

imprvhub commented Dec 22, 2024

@tpvasconcelos Thanks for pointing this out! Sounds promising - sphinxcontrib.apidoc seems to offer a more reliable approach. It's quite valuable in open source projects when we can explore different approaches to find the best solution together.

I'll make sure to dedicate the necessary time to analyze sphinxcontrib.apidoc in depth and see how we can adapt it to generate that toc-tree you mentioned for internal packages. In the meantime, feel free to share any additional thoughts or improvements you come up with. Thanks for your patience and guidance! 👍

@imprvhub take a look at a different approach using sphinxcontrib.apidoc here --> #296

I think that this approach is simpler and more reliable than what we are trying to do here. One thing missing from that approach is a nice way to render a similar toc-tree for the internal packages, but maybe we can think of a different solution/approach.

I'll sleep on this and try to see if I can think of a better solution. Feel free to take a look at it in the meantime :)

@tpvasconcelos
Copy link
Owner

@imprvhub sphinx-apidoc doesn't solve all the issues that I have encountered when trying to streamline the maintenance work around the API docs but it does solve the issue of automatically generating the API docs for the entire package. Since this was the main motivation for #290, I'm inclined to merge #297 and the original issue.

I think I need to spend some more time in the future looking at other packages in the Python OSS space to see how they do things but after a quick look I have not found any that does everything I want:

  1. Automatic API generation for entire package
  2. Leverage type annotations instead of re-documenting types in docstrings
  3. Resolve (cross-)references for type annotations
  4. Include references to parameters (accessible via :paramref:)
  5. Possibly others that I can't remember right now...

@tpvasconcelos
Copy link
Owner

Btw thanks for the effort in trying to solve this with me! 🙏

It sounds like a simple issue but sometimes the tooling just isn't there (or doesn't address all edge cases). Considering that this is such a common use-case, it is kinda crazy how partitioned the space is (tooling for Sphinx API docs).

@imprvhub
Copy link
Author

imprvhub commented Dec 23, 2024

@tpvasconcelos, thanks for your feedback! While sphinx-apidoc addresses (partially) your requirements for automatic API documentation generation (#290), I completely understand your points about the broader gaps in Python documentation tooling and the features we still need.

My initial PR relied on a custom RST generator, but sphinx-apidoc has proven to be a partial solution for you now. I haven't tested the sphinx-apidoc approach locally yet, but I'm glad you made some progress on this solution. If I think of any ways to improve this automation, I'll definitely keep you posted - maybe I can contribute more in the future when I have more time.

By the way, the plots are awesome! I'll be using ridgeplot from now on. Happy holidays colleague! 🎄

Btw thanks for the effort in trying to solve this with me! 🙏

It sounds like a simple issue but sometimes the tooling just isn't there (or doesn't address all edge cases). Considering that this is such a common use-case, it is kinda crazy how partitioned the space is (tooling for Sphinx API docs).

@imprvhub imprvhub closed this Dec 23, 2024
@imprvhub imprvhub deleted the feat/auto-generate-internal-docs branch December 23, 2024 02:15
@imprvhub imprvhub restored the feat/auto-generate-internal-docs branch December 23, 2024 02:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatically generate the docs/api/internal/*.rst files
2 participants