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

isidentifier() improvements #58278

Open
wants to merge 2 commits into
base: devel
from

Conversation

Projects
None yet
3 participants
@mkrizek
Copy link
Contributor

commented Jun 24, 2019

SUMMARY

When working on #58062 I discovered that True, False and None are not considered as keywords on Python 2 (why the CI fails on the mentioned PR):

Python 2.7.16 (default, Jun  6 2019, 10:58:02) 
>>> from ansible.utils.vars import isidentifier
>>> isidentifier('True')
True
Python 3.7.3 (default, Jun  6 2019, 11:02:20)
>>> from ansible.utils.vars import isidentifier
>>> isidentifier('True')
False

This PR changes implementation of the isidentifier function:

  • introducing two different implementations for Python 2 and Python 3, again (as the original implementation) taken from https://stackoverflow.com/a/29586366. The reason for two implementations is that Python 3's str.isdentifier that is used results in performance gain that will be useful for #58062 where we will check valid variable names for each variable that 'arrives in Ansible'. Python 2's version is also faster than the original one. See measurement results below.
  • for the Python 2 implementation adding ('True', 'False', 'None') to keywords unifying behavior for both Python 2 and 3,
  • adding unit tests

Note that the changes are made in lib/ansible/utils/vars.py. The functionality will be moved to lib/ansible/vars/validation.py in #58062.

For perf testing I used this basic script:

import timeit
from ansible.utils.vars import isidentifier

# testing valid identifier which is not a keyword which should be the worst case for the function
TEST = "isidentifier('_Ansible2point9_2019')"
SETUP = "from __main__ import isidentifier"

for number in (10000, 100000, 1000000):
    res = timeit.timeit(TEST, setup=SETUP, number=number)
    print("{} calls in {}".format(number, res))

This PR:

Python 3.7.3
10000 calls in 0.0029089649906381965
100000 calls in 0.029362355009652674
1000000 calls in 0.294429208006477

Python 2.7.16
10000 calls in 0.0076100826263427734
100000 calls in 0.07663202285766602
1000000 calls in 0.7761940956115723
After d723557
10000 calls in 0.0112590789795
100000 calls in 0.129191875458
1000000 calls in 1.13052797318

Currently in devel:

Python 3.7.3
10000 calls in 0.03599851200124249
100000 calls in 0.36006217898102477
1000000 calls in 3.6374873600143474

Python 2.7.16
10000 calls in 0.03914690017700195
100000 calls in 0.39107489585876465
1000000 calls in 3.880436897277832

ISSUE TYPE
  • Bugfix Pull Request
  • Feature Pull Request
COMPONENT NAME

lib/ansible/utils/vars.py

@mkrizek

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

Show resolved Hide resolved lib/ansible/utils/vars.py Outdated
Show resolved Hide resolved lib/ansible/utils/vars.py Outdated
Show resolved Hide resolved lib/ansible/utils/vars.py Outdated

@mkrizek mkrizek force-pushed the mkrizek:isidentifier branch from 575963a to 71281a7 Jun 25, 2019

@mkrizek mkrizek requested a review from bcoca Jun 25, 2019

@mkrizek mkrizek changed the title [WIP] isidentifier() improvements isidentifier() improvements Jun 25, 2019

@mkrizek mkrizek removed the WIP label Jun 25, 2019

@ansibot ansibot added the core_review label Jun 25, 2019

Show resolved Hide resolved lib/ansible/utils/vars.py Outdated

@mkrizek mkrizek requested a review from bcoca Jun 26, 2019

@bcoca

bcoca approved these changes Jun 26, 2019

@ansibot ansibot removed the needs_revision label Jun 26, 2019

@ansibot ansibot added the core_review label Jun 26, 2019

@ansibot ansibot added the stale_ci label Jul 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.