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

use faster method to get short_description from DOCUMENTATION #42705

Merged
merged 3 commits into from
Jul 27, 2018

Conversation

erinish
Copy link
Contributor

@erinish erinish commented Jul 12, 2018

SUMMARY

Created a stub lookup function for pulling only the short_description from docstrings to speed up the runtime of the ansible-doc -l command.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

cli/doc

ANSIBLE VERSION
ansible 2.7.0.dev0 (faster_docstring_lookups 35a7843ffc) last updated 2018/07/12 14:16:52 (GMT -400)
  config file = None
  configured module search path = [u'/root/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /root/code/hacking/ansible/lib/ansible
  executable location = /usr/bin/ansible
  python version = 2.7.5 (default, Aug  4 2017, 00:39:18) [GCC 4.8.5 20150623 (Red Hat 4.8.5-16)]

ADDITIONAL INFORMATION

The idea is to not use the heavy portions of get_docstring when the only piece needed is the short_description for output to ansible-doc -l. The current lookup is parsing every module and converting them to nodes, which is slow operation that will continue to grow over time as modules grow or are added.

I have made a couple assumptions: documentation string doesn't have leading indents, and the first instance of short_description in a given module is the one in the documentation string.

Given the style guide for module creation I don't think it would be an issue, but happy to make handling more robust if there are known cases where this wouldn't be enforced (out put of command is unchanged on my test system with the new method).

I also implemented this as an additional parser function for get_plugin_list_text in case this was used or planned to be used elsewhere in its current state. If this is the only function it performs then get_docstub doesn't need to be parameterized in the function call and can just replace get_docstring.

# ansible-doc -l | md5sum
4a157e4253a1adada4b69fe0016c9690  -
# git checkout faster_docstring_lookups
Switched to branch 'faster_docstring_lookups'
# ansible-doc -l | md5sum
4a157e4253a1adada4b69fe0016c9690  -

@ansibot ansibot added affects_2.7 This issue/PR affects Ansible v2.7 feature This issue/PR relates to a feature request. needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Jul 12, 2018
@samdoran
Copy link
Contributor

This is a very nice improvement when running ansible-doc -l:

(devel)> time ansible-doc -l > /dev/null
       30.37 real        29.89 user         0.37 sys

(PR/42705-erinish/faster_docstring_lookups)> time ansible-doc -l > /dev/null
        2.49 real         1.79 user         0.33 sys

@samdoran samdoran removed the needs_triage Needs a first human triage before being processed. label Jul 12, 2018
Copy link
Member

@nitzmahone nitzmahone left a comment

Choose a reason for hiding this comment

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

LGTM; nice speedup, and I can't really think of a way to break it the way we do plugin docs today... The CI sanity test is no longer using -l for the "full parse test", so I'm +1 for this.

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jul 22, 2018
@bcoca bcoca merged commit 551501f into ansible:devel Jul 27, 2018
@abadger
Copy link
Contributor

abadger commented Aug 23, 2018

I also implemented this as an additional parser function for get_plugin_list_text in case this was used or planned to be used elsewhere in its current state. If this is the only function it performs then get_docstub doesn't need to be parameterized in the function call and can just replace get_docstring.

I needed to retrieve the metadata for modules which failed because of missing documentation and thought I should mention at the same time: you were correct about this not needing to be implemented as a parameter in the function call. Just replacing get_docstring would be a much simpler and better approach (especially as it doesn't set up a contract that it will never fulfill (all return values being empty except for doc). I've modified the code so that it follows the simpler pattern.

@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.7 This issue/PR affects Ansible v2.7 feature This issue/PR relates to a feature request. new_contributor This PR is the first contribution by a new community member. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants