[PATCH 0/3] python/notmuch2: a few docstrings and collect_tags()

[PATCH 0/3] python/notmuch2: a few docstrings and collect_tags()

[Michael J Gruber]

collect_tags() (collecting all tags from messages in a query) is
available in the legacy bindings. Patch 3/3 provides it in the cffi
bindings. Patch 1 and 2 are doctsrings clean-ups/amends.

On 3/3: The tag iterator returned by the underlying library function is
short lived. If I simply return the ImmutableTagSet then it is empty
after one call. The way I make it long lived is nothing that I am proud
of - there may be much better ways. But it feels wrong to return an
"ImmutableTagSet" which survives only one call; it's not an iterator.

Not exposing queries nor "messages objects" seems to be a design
decision of the cffi bindings, and this patch follows it.

[resent since this didn't show up on the ml archive in a week]
[resent again after subscribing; maybe the list config changed, though
doc did not]

Michael J Gruber (3):
  python/notmuch2: correct docstring for Database.count_messages()
  python/notmuch2: docstrings for Database.threads(),
    Database.count_threads()
  python/notmuch2: provide binding for collect_tags()

 bindings/python-cffi/notmuch2/_database.py | 41 ++++++++++++++++++++--
 bindings/python-cffi/notmuch2/_query.py    | 12 +++++++
 2 files changed, 50 insertions(+), 3 deletions(-)

-- 
2.30.0.rc0.297.gbcca948854

[PATCH 1/3] python/notmuch2: correct docstring for Database.count_messages()

[Michael J Gruber]

Signed-off-by: Michael J Gruber <git@grubix.eu>
---
 bindings/python-cffi/notmuch2/_database.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/bindings/python-cffi/notmuch2/_database.py b/bindings/python-cffi/notmuch2/_database.py
index 868f4408..31b282f6 100644
--- a/bindings/python-cffi/notmuch2/_database.py
+++ b/bindings/python-cffi/notmuch2/_database.py
@@ -605,10 +605,10 @@ class Database(base.NotmuchObject):
                        omit_excluded=EXCLUDE.TRUE,
                        sort=SORT.UNSORTED,  # Check this default
                        exclude_tags=None):
-        """Search the database for messages.
+        """Search the database for messages and count.
 
-        :returns: An iterator over the messages found.
-        :rtype: MessageIter
+        :returns: The number of messages found.
+        :rtype: int
 
         :raises ObjectDestroyedError: if used after destroyed.
         """
-- 
2.30.0.rc0.297.gbcca948854

[PATCH 2/3] python/notmuch2: docstrings for Database.threads(), Database.count_threads()

[Michael J Gruber]

Signed-off-by: Michael J Gruber <git@grubix.eu>
---
 bindings/python-cffi/notmuch2/_database.py | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/bindings/python-cffi/notmuch2/_database.py b/bindings/python-cffi/notmuch2/_database.py
index 31b282f6..7592956a 100644
--- a/bindings/python-cffi/notmuch2/_database.py
+++ b/bindings/python-cffi/notmuch2/_database.py
@@ -622,6 +622,15 @@ class Database(base.NotmuchObject):
                 omit_excluded=EXCLUDE.TRUE,
                 sort=SORT.UNSORTED,  # Check this default
                 exclude_tags=None):
+        """Search the database for threads.
+
+        :returns: An iterator over the threads found.
+        :rtype: ThreadIter
+
+        :raises OutOfMemoryError: if no memory is available to
+           allocate the query.
+        :raises ObjectDestroyedError: if used after destroyed.
+        """
         query = self._create_query(query,
                                    omit_excluded=omit_excluded,
                                    sort=sort,
@@ -632,6 +641,15 @@ class Database(base.NotmuchObject):
                       omit_excluded=EXCLUDE.TRUE,
                       sort=SORT.UNSORTED,  # Check this default
                       exclude_tags=None):
+        """Search the database for threads and count.
+
+        :returns: The number of threads found.
+        :rtype: int
+
+        :raises OutOfMemoryError: if no memory is available to
+           allocate the query.
+        :raises ObjectDestroyedError: if used after destroyed.
+        """
         query = self._create_query(query,
                                    omit_excluded=omit_excluded,
                                    sort=sort,
-- 
2.30.0.rc0.297.gbcca948854

[PATCH 3/3] python/notmuch2: provide binding for collect_tags()

[Michael J Gruber]

collect_tags() is accessible in the legacy bindings as a method on the
messages object returned by searches. In the cffi bindings, neither
queries nor their internal values are exposed, so provide a binding
analogous to count_messages().

Signed-off-by: Michael J Gruber <git@grubix.eu>
---
 bindings/python-cffi/notmuch2/_database.py | 17 +++++++++++++++++
 bindings/python-cffi/notmuch2/_query.py    | 12 ++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/bindings/python-cffi/notmuch2/_database.py b/bindings/python-cffi/notmuch2/_database.py
index 7592956a..f3452e11 100644
--- a/bindings/python-cffi/notmuch2/_database.py
+++ b/bindings/python-cffi/notmuch2/_database.py
@@ -618,6 +618,23 @@ class Database(base.NotmuchObject):
                                    exclude_tags=exclude_tags)
         return query.count_messages()
 
+    def collect_tags(self, query, *,
+                       omit_excluded=EXCLUDE.TRUE,
+                       sort=SORT.UNSORTED,  # Check this default
+                       exclude_tags=None):
+        """Search the database for messages and collect tags.
+
+        :returns: The tags of all messages found.
+        :rtype: ImmutableTagset
+
+        :raises ObjectDestroyedError: if used after destroyed.
+        """
+        query = self._create_query(query,
+                                   omit_excluded=omit_excluded,
+                                   sort=sort,
+                                   exclude_tags=exclude_tags)
+        return query.collect_tags()
+
     def threads(self,  query, *,
                 omit_excluded=EXCLUDE.TRUE,
                 sort=SORT.UNSORTED,  # Check this default
diff --git a/bindings/python-cffi/notmuch2/_query.py b/bindings/python-cffi/notmuch2/_query.py
index 1db6ec96..a1310944 100644
--- a/bindings/python-cffi/notmuch2/_query.py
+++ b/bindings/python-cffi/notmuch2/_query.py
@@ -2,6 +2,7 @@ from notmuch2 import _base as base
 from notmuch2 import _capi as capi
 from notmuch2 import _errors as errors
 from notmuch2 import _message as message
+from notmuch2 import _tags as tags
 from notmuch2 import _thread as thread
 
 
@@ -66,6 +67,17 @@ class Query(base.NotmuchObject):
             raise errors.NotmuchError(ret)
         return count_p[0]
 
+    def collect_tags(self):
+        """Return the tags of messages matching this query."""
+        msgs_pp = capi.ffi.new('notmuch_messages_t**')
+        ret = capi.lib.notmuch_query_search_messages(self._query_p, msgs_pp)
+        if ret != capi.lib.NOTMUCH_STATUS_SUCCESS:
+            raise errors.NotmuchError(ret)
+        self._msgs_p = msgs_pp[0]
+        tagset = tags.ImmutableTagSet(
+            self, '_msgs_p', capi.lib.notmuch_messages_collect_tags)
+        return tags.ImmutableTagSet._from_iterable(tagset)
+
     def threads(self):
         """Return an iterator over all the threads found by the query."""
         threads_pp = capi.ffi.new('notmuch_threads_t **')
-- 
2.30.0.rc0.297.gbcca948854

Re: [PATCH 0/3] python/notmuch2: a few docstrings and collect_tags()

[David Bremner]

Michael J Gruber <git@grubix.eu> writes:

> collect_tags() (collecting all tags from messages in a query) is
> available in the legacy bindings. Patch 3/3 provides it in the cffi
> bindings. Patch 1 and 2 are doctsrings clean-ups/amends.

I'm hoping will review your patches, but I can at least confirm they
reached the list. Of course you probably saw them in the archive as
well.

Re: [PATCH 3/3] python/notmuch2: provide binding for collect_tags()

[Floris Bruynooghe]

Hi Micahel,

Thanks for adding this feature!

On Wed 06 Jan 2021 at 10:08 +0100, Michael J. Gruber wrote:
> diff --git a/bindings/python-cffi/notmuch2/_query.py b/bindings/python-cffi/notmuch2/_query.py
> index 1db6ec96..a1310944 100644
> --- a/bindings/python-cffi/notmuch2/_query.py
> +++ b/bindings/python-cffi/notmuch2/_query.py
> @@ -2,6 +2,7 @@ from notmuch2 import _base as base
>  from notmuch2 import _capi as capi
>  from notmuch2 import _errors as errors
>  from notmuch2 import _message as message
> +from notmuch2 import _tags as tags
>  from notmuch2 import _thread as thread
>  
>  
> @@ -66,6 +67,17 @@ class Query(base.NotmuchObject):
>              raise errors.NotmuchError(ret)
>          return count_p[0]
>  
> +    def collect_tags(self):
> +        """Return the tags of messages matching this query."""
> +        msgs_pp = capi.ffi.new('notmuch_messages_t**')
> +        ret = capi.lib.notmuch_query_search_messages(self._query_p, msgs_pp)
> +        if ret != capi.lib.NOTMUCH_STATUS_SUCCESS:
> +            raise errors.NotmuchError(ret)
> +        self._msgs_p = msgs_pp[0]
> +        tagset = tags.ImmutableTagSet(
> +            self, '_msgs_p', capi.lib.notmuch_messages_collect_tags)
> +        return tags.ImmutableTagSet._from_iterable(tagset)
> +
>      def threads(self):
>          """Return an iterator over all the threads found by the query."""
>          threads_pp = capi.ffi.new('notmuch_threads_t **')

How this this look on the C level for the memory allocator?  If the
query gets freed the messages also get freed?  In that case indeed the
messages do need to be kept alive together with the query indeed.
Keeping them as an attribute on the query object seems reasonable I
think, they'd be freed at the same time and I don't think this creates a
circular reference.

There's only a small detail though: to get the correct ObjectDestroyed
exceptions you need Query._msgs_p to be a _base.MemoryPoiniter
descriptor, like Query._query_p is currently.  And then you also need to
set it to None in Query._destroy.

It would also be really good to add at least one happy-path test for
this, ensuring that you get the tags in the query.  You could add some
to TestQuery in test_database.py.  The database is created with unread
and inbox tags i think (see conftest.py:75) so the messages should
already have some tags which is sufficient as you're not testing
notmuch's behaviour itself but only that the bindings give you a right
looking tagset (the tagset itself also has tests already, so no need to
test this extensively, just see if you have some tags).

Otherwise LGTM!

Cheers,
Floris

Re: [PATCH 0/3] python/notmuch2: a few docstrings and collect_tags()

[Floris Bruynooghe]

On Thu 07 Jan 2021 at 17:09 +0000, Michael J. Gruber wrote:
> As for the series: the notmuch based MUA "alot" switched to the new
> python bindings recently. collect_tags() is something I used in a
> feature PR submitted but not merged yet there (while on the old bindings),
> and in my updated feature PR there I directly roll
> notmuch2._tags.ImmutableTagSet(msgs, '_iter_p', notmuch2.capi.lib.notmuch_messages_collect_tags).

You could do this entirely in the public bindings too could you not?
Something like (untested):

def collect_tags(db, query):
    tags = set()
    for msg in db.messages(query):
        tags.update(msg.tags)

anyway, i guess the internal APIs you use won't change before your
patchset here lands so this doesn't matter much.


> I don't know whether this will land in alot, but feature parity of the
> new bindings with the old ones is a good aim

Thanks for contributing this!  I never aimed for full parity because I
didn't feel like making the API decisions for APIs I had no need to use
myself.  But it's great when people needing things can add it!


Cheers,
Floris

Re: [PATCH 0/3] python/notmuch2: a few docstrings and collect_tags()

[Michael J Gruber]

Floris Bruynooghe venit, vidit, dixit 2021-01-11 21:47:33:
> On Thu 07 Jan 2021 at 17:09 +0000, Michael J. Gruber wrote:
> > As for the series: the notmuch based MUA "alot" switched to the new
> > python bindings recently. collect_tags() is something I used in a
> > feature PR submitted but not merged yet there (while on the old bindings),
> > and in my updated feature PR there I directly roll
> > notmuch2._tags.ImmutableTagSet(msgs, '_iter_p', notmuch2.capi.lib.notmuch_messages_collect_tags).
> 
> You could do this entirely in the public bindings too could you not?
> Something like (untested):
> 
> def collect_tags(db, query):
>     tags = set()
>     for msg in db.messages(query):
>         tags.update(msg.tags)
> 
> anyway, i guess the internal APIs you use won't change before your
> patchset here lands so this doesn't matter much.

Yes, that gives the same set.

A quick unscientific test on my 80k messages db with 640 different tags
gives for the search string '*':

notmuch search --ouput=tags: < 0.01s (shell's time)
nm2.db.tags (can do '*' only): < 1e-4s
direct roll as quoted above: 0.02s
from public bindings as above: 4.5s
(all py times time.perf_counter, python 2.9)

To be honest, converting the result of the direct roll to a set or list adds 2s.
So, depending on the query and use case, it can be a tremendous
difference or almost unnoticable.

Cheers
Michael

Re: [PATCH 3/3] python/notmuch2: provide binding for collect_tags()

[Michael J Gruber]

Floris Bruynooghe venit, vidit, dixit 2021-01-11 21:33:16:
> Hi Micahel,
> 
> Thanks for adding this feature!
> 
> On Wed 06 Jan 2021 at 10:08 +0100, Michael J. Gruber wrote:
> > diff --git a/bindings/python-cffi/notmuch2/_query.py b/bindings/python-cffi/notmuch2/_query.py
> > index 1db6ec96..a1310944 100644
> > --- a/bindings/python-cffi/notmuch2/_query.py
> > +++ b/bindings/python-cffi/notmuch2/_query.py
> > @@ -2,6 +2,7 @@ from notmuch2 import _base as base
> >  from notmuch2 import _capi as capi
> >  from notmuch2 import _errors as errors
> >  from notmuch2 import _message as message
> > +from notmuch2 import _tags as tags
> >  from notmuch2 import _thread as thread
> >  
> >  
> > @@ -66,6 +67,17 @@ class Query(base.NotmuchObject):
> >              raise errors.NotmuchError(ret)
> >          return count_p[0]
> >  
> > +    def collect_tags(self):
> > +        """Return the tags of messages matching this query."""
> > +        msgs_pp = capi.ffi.new('notmuch_messages_t**')
> > +        ret = capi.lib.notmuch_query_search_messages(self._query_p, msgs_pp)
> > +        if ret != capi.lib.NOTMUCH_STATUS_SUCCESS:
> > +            raise errors.NotmuchError(ret)
> > +        self._msgs_p = msgs_pp[0]
> > +        tagset = tags.ImmutableTagSet(
> > +            self, '_msgs_p', capi.lib.notmuch_messages_collect_tags)
> > +        return tags.ImmutableTagSet._from_iterable(tagset)
> > +
> >      def threads(self):
> >          """Return an iterator over all the threads found by the query."""
> >          threads_pp = capi.ffi.new('notmuch_threads_t **')
> 
> How this this look on the C level for the memory allocator?  If the
> query gets freed the messages also get freed?  In that case indeed the
> messages do need to be kept alive together with the query indeed.
> Keeping them as an attribute on the query object seems reasonable I
> think, they'd be freed at the same time and I don't think this creates a
> circular reference.
 
I'm afraid I don't have the full picture of the memory model and object
lifetimes in the bindings.

> There's only a small detail though: to get the correct ObjectDestroyed
> exceptions you need Query._msgs_p to be a _base.MemoryPoiniter
> descriptor, like Query._query_p is currently.  And then you also need to
> set it to None in Query._destroy.

I was trying to mimick what Query.messages() does: it creates msgs_pp
locally. The only reason for self._msgs_p in Thread.collect_tags() is
the way in which tags.ImmutableTagSet() expects the pointer: as an
attribute of self. But maybe there is a better way to call into
capi.lib.notmuch_messages_collect_tags()?

If not, and if I understand you correctly, then I should apply

diff --git i/bindings/python-cffi/notmuch2/_query.py w/bindings/python-cffi/notmuch2/_query.py
index a1310944..fa58825d 100644
--- i/bindings/python-cffi/notmuch2/_query.py
+++ w/bindings/python-cffi/notmuch2/_query.py
@@ -17,6 +17,7 @@ class Query(base.NotmuchObject):
     match libnotmuch's memory management.
     """
     _query_p = base.MemoryPointer()
+    _msgs_p = base.MemoryPointer()

     def __init__(self, db, query_p):
         self._db = db
@@ -40,6 +41,7 @@ class Query(base.NotmuchObject):
         if self.alive:
             capi.lib.notmuch_query_destroy(self._query_p)
         self._query_p = None
+        self._msgs_p = None

on top of this patch, right?


> It would also be really good to add at least one happy-path test for
> this, ensuring that you get the tags in the query.  You could add some
> to TestQuery in test_database.py.  The database is created with unread
> and inbox tags i think (see conftest.py:75) so the messages should
> already have some tags which is sufficient as you're not testing
> notmuch's behaviour itself but only that the bindings give you a right
> looking tagset (the tagset itself also has tests already, so no need to
> test this extensively, just see if you have some tags).

Thanks for the pointer. Will do for v2, comparing:
set(db.collect_tags('*')) == set(db.tags)

Michael