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

Paginator docs #645

Merged
merged 4 commits into from
Sep 18, 2015
Merged

Paginator docs #645

merged 4 commits into from
Sep 18, 2015

Conversation

kyleknap
Copy link
Member

@kyleknap kyleknap commented Sep 4, 2015

Add docstrings to paginator's paginate method. It also renames the paginator class returned from client to a class that falls in line with what is in the documentation e.g. S3.Paginator.list_objects.

In [1]: import botocore.session

In [2]: session = botocore.session.get_session()

In [3]: client = session.create_client('s3')

In [4]: paginator = client.get_paginator('list_objects')

In [5]: paginator.__class__.__name__
Out[5]: 'S3.Paginator.list_objects'

In [6]: help(paginator.paginate)

I realize that I have a lot of PRs out right now and many depend on the other. This is the order in which how I would like to see them reviewed/merged:

  1. Some moderate refactoring for boto3 docstrings #642
  2. Waiter docstrings #643
  3. This one
  4. Add docstrings to resource actions and sub-resources boto3#239

Or you can just review this one, which accounts for 1 through 3.

cc @jamesls @mtdowling @rayluo

@jamesls
Copy link
Member

jamesls commented Sep 11, 2015

I primarily have the same question as the waiter doc PR. I'm not sure if we want to create a new class on every single call to get_paginator. I know do that for session.create_client, but I would envision a paginator/waiter within a client to be called much more than a create_client call.

@kyleknap
Copy link
Member Author

Doing some performance testing with the paginators as well. Here are some preliminary results in terms of speed. I used this script:

import time
import botocore.session

session = botocore.session.get_session()
s3 = session.create_client('s3')

total = 0
iterations = 5
for i in range(iterations):
    start_time = time.time()
    s3.get_paginator('list_objects')
    end_time = time.time()
    diff = end_time - start_time
    print('%s round instantiation: %s seconds' % (i, diff))
    total += diff

average = total/iterations
print('Average %f seconds' % average)

For the develop branch, I got these results:

0 round instantiation: 0.00303792953491 seconds
1 round instantiation: 3.79085540771e-05 seconds
2 round instantiation: 2.40802764893e-05 seconds
3 round instantiation: 3.19480895996e-05 seconds
4 round instantiation: 4.10079956055e-05 seconds
Average 0.000635 seconds

For my PR I got:

0 round instantiation: 0.0030620098114 seconds
1 round instantiation: 8.98838043213e-05 seconds
2 round instantiation: 6.29425048828e-05 seconds
3 round instantiation: 6.69956207275e-05 seconds
4 round instantiation: 6.00814819336e-05 seconds
Average 0.000668 seconds

So in terms of speed it looks like it was only affected by a little.

@kyleknap
Copy link
Member Author

So I ran some memory leak tests similiarly to what I did for the waiter docstring pull request. Here is my results for s3 ListObjects paginator.

I ran each test five times and took the average difference between the end memory usage and the beginning memory usage (i.e. end - start):

develop test_create_single_paginator_memory_constant: 4.46 MB
develop test_create_memory_paginators_in_loop: 1.63 MB

PR test_create_single_paginator_memory_constant: 4.04 MB
PR test_create_memory_paginators_in_loop: 1.93 MB

Surprisingly the memory leakage decreased for one of the tests using my PR. Similarly to the Waiter docstring PR, I do not think we need to cache the Paginator class. Thoughts?

@kyleknap kyleknap added the pr/needs-review This PR needs a review from a member of the team. label Sep 14, 2015
Also renamed the paginator class name to fall in line with what is in
the documentation for a specific paginator.
Change was made to be more pythonic and consistent with Waiter class names.
@jamesls
Copy link
Member

jamesls commented Sep 18, 2015

:shipit:

kyleknap added a commit that referenced this pull request Sep 18, 2015
@kyleknap kyleknap merged commit a860b7e into boto:develop Sep 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/needs-review This PR needs a review from a member of the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants