add stdlib queue.Queue compatible API by Kriechi · Pull Request #7 · philipbl/python-persistent-queue

Kriechi · 2016-11-14T19:29:52Z

This PR adds new functions to reach API-compatibility with the Python stdlib queue.Queue.
I'll leave bumping the major version number to @philipbl.

fixes #2

ToDo:

add tests for new API function
add timeout handling to get and put
restructure API to Python stdlib queue
add maxsize checks to handle full queues
make use of queue.Empty and queue.Full exceptions
100% test coverage
add documentation

philipbl · 2016-11-15T15:58:41Z

Why did you switch to py.test?

Kriechi · 2016-11-15T17:35:53Z

You already invoked the tests using py.test:

python-persistent-queue/.travis.yml

Line 10 in e319e5e

- "py.test -v --cov persistent_queue"

So I though it would be nice to fully embrace the py.test goodness(tm). 😄

What do you think in general of the proposed API additions?

philipbl · 2016-11-16T19:40:11Z

The API changes seem good!

I think we could remove some redundant methods. For example, instead of having a push method, we could use put, but have it handle multiple values, similar to how push currently works. Instead of having a pop method, we could use the get method, but change it to take an optional value for the number of items to get. These changes would still be Queue compatible but remove the extra methods (and possibly confusing terms: pop vs get, push vs put).

philipbl · 2016-11-16T19:41:59Z

Could you separate the changes to the tests into another PR?

Kriechi · 2016-11-16T19:43:41Z

Sure! I didn't want to mess with your existing API too much (semver and all...)
But if you are open to bumping up to 2.0 or something, I would be happy to make the stdlib API "the default". Should I open another PR - or should I integrate it here?

Kriechi · 2016-11-16T19:44:58Z

Could you separate the changes to the tests into another PR?

Done.

coveralls · 2016-11-16T19:50:02Z

Coverage decreased (-9.4%) to 90.61% when pulling 3bf0c05 on Kriechi:queue-api into e319e5e on philipbl:master.

philipbl · 2016-11-17T20:21:27Z

Yeah, let's bump it to version 2. I think it would be better to have the API resemble something that people are familiar with.

You can make those changes as part of this PR. What ever is easiest.

Kriechi · 2016-11-17T23:32:48Z

Ok - I just pushed most of the refactoring into this PR. I might have gone overboard and changed a lot of things. If you have any objections to the specifics parts, I will try to extract it.

use tox to have a nice test matrix
use flake8 as linter
use modern package layout with __init__.py
implement stdlib-queue-compatible API
restructure class and expand doc-comments
fix a potential bug with self.path, use os.abspath to get a full filename
mark all internal members as private with self._foobar underscore
merged / reimplemented python2.7 support from master

Two tests are still missing - then we have 100% coverage again.

Kriechi · 2016-11-18T10:59:56Z

@philipbl all done!
Let me know if you are happy with the API, then I will start working on the README / documentation.

philipbl · 2016-11-26T07:11:44Z

Sorry for the slow response. I really like it!

I have a few comments, but it's too late tonight and I want to look at the code again in the morning.

Thanks for all of your work on this!

philipbl

I left a few comments for you. Let me know what you think.

philipbl · 2016-11-30T20:55:11Z

+                        if timeout <= 0:
+                            raise queue.Empty
+                self._put_event.clear()
+        elif not partial and items == 1:


What should happen if items > 1? For example, queue.get(block=False, items=2).

I guess this part is undefined at the moment. What would you expect it to do? Should it raise if we try to get more items than are available, or just return everything that is available?

It probably doesn't matter as long as it is consistent and documented.

I think when you peek (partial=True), the data that is available should be given, even if more was requested. When you get (partial=False), an exception should be raised if there is not enough items in the queue, no matter how many items are being requested.

So I would change lines 130-132 to be:

elif not partial and self._length < items: raise queue.Empty

philipbl · 2016-11-30T21:08:37Z

+            if timeout is not None:
+                target = time.time() + timeout
+            while self._length < items:
+                if self._put_event.wait(timeout) is not True:


Maybe I don't completely understand, but the logic doesn't seem to be right. First, shouldn't the not True be changed to True? Second, if wait returns True, then we need to check the length again before we see if the timeout has expired and raise an exception. Something like:

while self._length < items: if self._put_event.wait(timeout) is True: if timeout is not None: timeout = target - time.time() if self._length < items and timeout <= 0: raise queue.Empty self._put_event.clear()

Third, we need something to handle the case where wait returns False:

if self._put_event.wait(timeout) is True: ... else: raise queue.Empty

This part is within the if block: condition - so we should not raise a queue.Empty unless a timeout value is given.

If timeout == None, the code looks like this (simplified):

while self._length < items: self._put_event.wait(None) # only returns True self._put_event.clear()

If timeout is a number, the code looks like this (simplified)
target is going to be e.g. 5 seconds in the future.

while self._length < items: if self._put_event.wait(5) is not True: timeout = target - time.time() if timeout <= 0: # only raise if timeout was set and has expired raise queue.Empty # else: the event obviously returned True, which means a new item was pushed # so we just clear the event, and let the while-condition decode what to do next self._put_event.clear()

Can't

if self._put_event.wait(timeout) is not True:

be simplified to

if self._put_event.wait(timeout) is False:

?

Maybe I am understanding Event.wait incorrectly. Doesn't it return False if the wait call times out? Assuming that a timeout value is given, this branch

if self._put_event.wait(timeout) is not True:

will only be taken if wait has timed out. Since wait has timed out, we can just raise queue.Empty, we don't need to do the rest of the logic you have. It is only when wait returns True (because something was added the the queue so the wait didn't time out) that you need to adjust timeout to a new value, as long as there's still not enough items in the queue.

Yes - this is probably a bit weirdly written. As far as I understood the API docs, it returns True if the event got set. But in the case of a timeout, the return value is not defined (I assume it is and will be False forever. Do you want me to clean that up and make it more intuitive?

Well, I think the behavior is incorrect, not unintuitive. Here is what I think the behavior should be

if block: if timeout is not None: target = time.time() + timeout while self._length < items: if self._put_event.wait(timeout) is False: # Nothing was added to the queue and timeout expired # This will never happen if timeout is None raise queue.Empty # Something was added to the queue # Update timeout, if necessary if timeout is not None: timeout = target - time.time() if timeout <= 0: raise queue.Empty self._put_event.clear() elif not partial and items == 1: if self._length < items: raise queue.Empty

What do you think?

Yes - you are right!

One minor thing:
I think we can even remove this block - since if the timeout would be negative, the wait should have already returned False.

if timeout <= 0: raise queue.Empty

What do you think?

I agree 👍

philipbl · 2016-11-30T21:15:10Z

+        with self._all_tasks_done:
+            unfinished = self._unfinished_tasks - items
+            if unfinished <= 0:
+                if unfinished < 0:


It's a style thing, but I don't like the nested checks on unfinished. How about something like:

if unfinished < 0: raise ValueError('task_done() called too many times') elif unfinished == 0: self._all_tasks_done.notify_all()

Ok - I will change it.
FYI: I just copied this block of code straight out of lib/python3.5/queue.py from the Python stdlib.

philipbl · 2016-11-30T21:19:25Z

+        flush_limit: below this filesize, flush() is a no-op.
+        """
+        if filename is None:
+            raise ValueError("filename must be set, e.g. '/my/path/important.queue'")


Any reason why you didn't make filename a required parameter? I know it is a break from the Queue API, but isn't this practically the same thing?

I think you are right, this is probably a valid point to break API-compatibility.
However, I'm not sure how to implement this - because putting filename as a first argument might lead to confusion, since the std-API expects an integer as API...
Or should we just verify that filename is a str?

In my opinion, we don't need to check to make sure filename is a str. We don't do that for the rest of the parameters and os.path.abspath(filename) will fail if filename is an int. I don't feel strongly about this though, so if you want to add the check, go ahead.

Here is what I think the signature should be:

def __init__(self, filename, maxsize=0, dumps=pickle.dumps, loads=pickle.loads, flush_limit=1048576):

philipbl · 2016-11-30T21:22:21Z

+                    if timeout is not None:
+                        target = time.time() + timeout
+                    while self._length + len(items) > self.maxsize:
+                        if self._get_event.wait(timeout) is not True:


See my comments for _peek. I think they apply here as well.

Kriechi · 2016-12-01T20:55:20Z

I just pushed a few new commits that should address all your comments.

Kriechi · 2016-12-06T19:30:57Z

Yeah! Thanks! 👍 🎉 👏

implement stdlib queue API

3bf0c05

philipbl mentioned this pull request Nov 15, 2016

Add support for Python 2 #8

Merged

Kriechi force-pushed the queue-api branch from 3bbd31d to 3bf0c05 Compare November 16, 2016 19:44

Kriechi added 15 commits November 17, 2016 21:49

use tox and linter

a0d17b6

restructure into file

65c4ed4

mark internal members as private

df13de7

reorder methods

c395324

simplify filename and path usage

9c4152e

move methods

0ed3aba

implement queue.get

ab0f6c3

adapt tests

86e84d6

implement queue.put

34b146d

adapt tests

ab85bc6

adapt travis changes

56db7c5

fix os.replace on py27

da7586a

add py27 compatibility

2e2c701

Merge branch 'master' into queue-api

28e283a

fix travis

3b9842c

Kriechi added 3 commits November 18, 2016 10:53

implement put timeout

30c1c01

test cleanup

92ac3bb

implement get timeout

9365a1f

Kriechi changed the title ~~[WIP] add stdlib queue.Queue compatible API~~ add stdlib queue.Queue compatible API Nov 18, 2016

add tests for task_done and join

6230352

Kriechi force-pushed the queue-api branch from 2641dbb to 6230352 Compare November 18, 2016 10:56

lint tests too

81a3eb2

philipbl reviewed Nov 30, 2016

View reviewed changes

change unfinished checks

cafd413

philipbl mentioned this pull request Dec 1, 2016

Pushing None will mess up the internal state #9

Open

Kriechi added 4 commits December 1, 2016 21:31

change init API

87cc620

change partial behaviour

2c50207

fix block+timeout behaviour

5d010b3

update tests and lint

d3b63fb

remove unreachable code

1b372ff

Kriechi force-pushed the queue-api branch from 98f0d3d to 1b372ff Compare December 2, 2016 22:15

philipbl merged commit a2f19d2 into philipbl:master Dec 6, 2016

Kriechi deleted the queue-api branch December 6, 2016 19:30

philipbl added this to the Version 2 milestone Dec 6, 2016

Conversation

Kriechi commented Nov 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philipbl commented Nov 15, 2016

Uh oh!

Kriechi commented Nov 15, 2016

Uh oh!

philipbl commented Nov 16, 2016

Uh oh!

philipbl commented Nov 16, 2016

Uh oh!

Kriechi commented Nov 16, 2016

Uh oh!

Kriechi commented Nov 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Nov 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philipbl commented Nov 17, 2016

Uh oh!

Kriechi commented Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kriechi commented Nov 18, 2016

Uh oh!

philipbl commented Nov 26, 2016

Uh oh!

philipbl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kriechi commented Dec 1, 2016

Uh oh!

Kriechi commented Dec 6, 2016

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Kriechi commented Nov 14, 2016 •

edited

Loading

Kriechi commented Nov 16, 2016 •

edited

Loading

coveralls commented Nov 16, 2016 •

edited

Loading

Kriechi commented Nov 17, 2016 •

edited

Loading