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

Networking #33

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Networking #33

wants to merge 5 commits into from

Conversation

Indiiigo
Copy link
Collaborator

To ensure 'global' ratelimiting of various endpoints:

  • replaced the ratelimit python package with RateLimitedPaginator (connectivity.py)
  • switched the encapsulation so that api.py no longer imports anything from connectivity.py

Copy link
Owner

@hide-ous hide-ous left a comment

Choose a reason for hiding this comment

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

Looks good! I have a few points I did not get, but the general structure is what I had in mind

@@ -4,7 +4,7 @@
import os
import sys

from pytangle.connectivity import Paginator
#from pytangle.connectivity import RateLimitedPaginator
Copy link
Owner

Choose a reason for hiding this comment

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

best to remove the lines commented out, just for the sake of cleanliness

#yield from Paginator(endpoint=PostsEndpoint(args=remove_null_values_from_dict(params)))
#yield from RateLimitedPaginator(PostsEndpoint(args = remove_null_values_from_dict(params)))
#return PostsEndpoint.call_rate_limited_paginator(args = remove_null_values_from_dict(params))
yield from PostsEndpoint(args = remove_null_values_from_dict(params))
Copy link
Owner

Choose a reason for hiding this comment

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

nice and clean

@@ -9,29 +9,38 @@
from sys import exit
from dateutil.parser import parse as date_parse
import requests
from ratelimit import limits, sleep_and_retry, RateLimitException

from ratelimit import sleep_and_retry, RateLimitException
Copy link
Owner

Choose a reason for hiding this comment

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

this shouldn't be needed, right?

# ONE_MINUTE = 60
# TEN_SECONDS = 10
# THIRTY_SECONDS = 30
MAX_RETRIES = read_max_retries(read_config()) if read_max_retries(read_config()) else 5
Copy link
Owner

Choose a reason for hiding this comment

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

would be cleaner to read once from config and then adjusting the value, along the lines of

MAX_RETRIES = read_max_retries(read_config())
MAX_RETRIES = max(MAX_RETRIES, 5)

Copy link
Owner

Choose a reason for hiding this comment

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

is it actually used in the paginator?



# the following use of ratelimit should be removed
Copy link
Owner

Choose a reason for hiding this comment

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

please do :)

self.num_calls = 1 #num_calls
self.clock = time.time()
self.last_reset = time.time()
self.clamped_calls = max(1, min(sys.maxsize, floor(self.num_calls)))
Copy link
Owner

Choose a reason for hiding this comment

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

not sure what is happening here. could you guide me through?


self.request_fun = make_request # have make request here instead of importing in endpoints.py
self.time_unit = time_unit / num_calls
self.num_calls = 1 #num_calls
Copy link
Owner

Choose a reason for hiding this comment

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

should this be = num_calls?

Comment on lines +306 to +308
self.num_calls_so_far = 0
self.last_reset = time.time()

Copy link
Owner

Choose a reason for hiding this comment

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

would be easier down the line to have it made into a __reset() method


# If the number of attempts to call the function exceeds the
# maximum then raise an exception.
if self.num_calls_so_far / 100 >= self.clamped_calls and self.__period_remaining() > 0:
Copy link
Owner

Choose a reason for hiding this comment

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

I suspect that we would not need the check on __period_remaining . if it expires at this point it'd still be cleaner to throw the error, so that you can properly reset the num_calles_so_far counter

speaking of, why /100?

# If the number of attempts to call the function exceeds the
# maximum then raise an exception.
if self.num_calls_so_far / 100 >= self.clamped_calls and self.__period_remaining() > 0:
time.sleep(self.__period_remaining()) # sleep for time remaining
Copy link
Owner

Choose a reason for hiding this comment

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

sleep on negative values may throw errors; you could have a max(0, ...), or save the value returned at the start of the method in line 304

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.

2 participants