Replies: 13 comments 19 replies
-
|
Thanks. This is a good starting point. It will take some time until I can look into this.
I want a coherent design of the async pipeline. The composability of the consult--read async functions should transfer over to consult--multi, if possible. |
Beta Was this translation helpful? Give feedback.
-
|
Agree. Ideally we make it 100% backward compatible. I don't see why not - there shouldn't be a problem to automatically "upgrade" synchronous sources to asynchronous ones during the setup when generating the async function for consult--read. One small detail is maybe the consult--with-async macro in consult--read which performs setup only if the candidate table is async. If we always generate a composed async function, we may perform this setup unnecessarily. But shouldn't be difficult to avoid this. |
Beta Was this translation helpful? Give feedback.
-
Focusing on this specific example for a minute, I'm not sure I follow. In its current form, One narrowing enhancement possible with multi-async sources is to avoid fetching results for sources that are out of scope. In the above example, narrowing to Elfeed and then typing in a query (can) query only Elfeed, and not gptel and Brave. This cuts down on extraneous processes/network requests and their associated costs. I've made this adjustment to However, currently it doesn't query the other sources (gptel and Brave) automatically when I widen the source list, as there is no |
Beta Was this translation helpful? Give feedback.
-
I understand and appreciate this idea in the abstract, but I'm having trouble imagining what it looks like. Currently If it is updated to include async sources, it also has to make choices on how to build the async pipeline, or accept more arguments. I'm also unclear on how to build a different pipeline for each source. I'll wait for minad to work on it.
Yes, this part I follow. We'll need something cleverer than the current |
Beta Was this translation helpful? Give feedback.
-
|
@karthink Yeah, I am not sure which one is the better behavior for narrowing. May be you are right and we would always want all of the narrowing characters. But I think it might depends on how you use the consult-narrow. I think if you start with narrowing (e.g. typing "b + SPC") to narrow first, then in this scenario all narrow down options should be available and as you pointed the right behavior is probably to limit the scope in this case. On the other hand, if I run a search first and then use The more general point is that we should perhaps look at all of the features and ask wat makes sense in a general solution. For another example, grouping is somehting that may also need some thoughts. As far as I understand consult-multi by default groups candidates based on their source. But in my consult-web (using consult--read with dynamic collection) I change this so that grouping can be dynamically changed by passing arguments in the minibuffer (e.g. adding I think the right thing to do is exactly what you are doing now. Try things out with multi-source examples (like the example sources I already have in consult-web) to find out what general behaviors makes intuitive sense for narrowing, grouping, sorting, categories, ... Once I start doing some experiments, I can also add some more specific points for discussion here. |
Beta Was this translation helpful? Give feedback.
-
|
There are two sides of the discussion, the API side and the UI side. I think all the experiments will be valuable to get a better feel on how the UI should look like. Please proceed with that! Then we can try to fit all these features into a nice and coherent API, but this is really just an abstract idea right now.
Yes, it is an abstract idea right now. Overall the API won't be much different I hope. Maybe we need an additional argument to For example consider the pipeline from above: (consult--async-split
(consult--async-throttle
(consult--multi-async sources ;; The new multi async builder
(consult--async-refresh-timer
(consult--async-sink)))))
The pipeline from above would be created by this function:
I think we'll have some part of the pipeline shared (the transform from above) and some part of the pipeline individual per source. |
Beta Was this translation helpful? Give feedback.
-
|
@minad and @karthink another part that you two may have some ideas for is the whole async split concept of search and then narrow down (e.g. using perl syntax: This also affects the narrowing by narrow character as well. Right now since I am using the perl split syntax (which I love a lot), the The same applies to passing '-- --arg val" to async commands. What should one do with this part of the input in the case where some sources are sync and others are async? May be sync sources just ignore this part, but is that really the right thing to do? and what would get added to the history variable (only the search query or the whole query, args and narrow parts? There are cases where I like having the extra args in the history, but in other cases I only would want to keep the search query and almost never the narrowing part. May be at the end, we would still need other packages (like consult-web and consult-gh) to make some of these decisions per specific use-case but I think the general solution should at least have some coherent base that makes sense and allows the down stream applications to build what is needed around the default behavior. |
Beta Was this translation helpful? Give feedback.
-
Tbh I don't have a good idea. The input handling/splitting and the meaning of the input for mixed synchronous and asynchronous source is one of the initial reasons why I haven't added async multi sources in the past. Considering the perl splitter, I think both the first input and the second input should apply to all sources. The downside is that the first input is not using a completion style, so we lose a lot of functionality, e.g., from Orderless. Maybe the first input could also be treated differently, using the completion style for the synchronous sources and custom input handling for the asynchronous sources. This is however difficult to do since the completion style applies to all candidates. Another option would be to get rid of the perl splitter concept and instead use the completion-style processed input for both async and sync. Then we would lose the ability to do cheap post filtering, and asynchronous sources may not understand the regexps generated by the completion style. Let's list the problems so far:
Absolutely. The separate packages are needed in any cases since they will provide specialized sources not available in Consult itself. |
Beta Was this translation helpful? Give feedback.
-
@minad My point was not that the other packages will be needed or not, but rather that some of these decisions may need to be made within those packages and not in consult. For example, I would not get rid of the perl splitting because I find it very useful and the cheap filtering is essential for sources where one has thousands of candidates (like my consult-mu). I think doing this with completion styles would not be the right choice because then configurations would also be complicated (i.e. Consult's feature would directly depend on orderless config, etc.) So instead of making that decision in consult, may be the consult-* package/command has to decide whether a specific sync/async source uses both parts of the input or only one part. In other words within consult all parts of the input are sent to the fetcher command (async or sync) and then within the source definition one may need to parse the input (e.g. with consult--split-perl) and use different parts accordingly. This is the kind of thing that I was afraid would break backward compatibility since synchronous sources right now are not designed to parse a perl-style input, and therefore we may want to think about whether there is a better way to handle it without breaking what is already in place! |
Beta Was this translation helpful? Give feedback.
-
|
I have to think more about this. Static sources should not have to do any kind of input handling, but the returned candidates should be completion style filtered according to the input. Meanwhile for asynchronous sources the input should be used to start an asynchronous computation, returning only matching candidates. Then asynchronous sources also need second level completion style filtering. It is not really clear to me how we can reconcile these concepts. Maybe synchronous sources should do their own filtering as soon as they are combined with asynchronous sources. This would happen automatically during the consult--multi-async merging. Ideally we also somehow preserve the power and flexibility of the completion style, since I wouldn't want to lose Orderless. In its newest release it even got annotation and keyword filtering. |
Beta Was this translation helpful? Give feedback.
-
|
I made a standalone package as a full proof of concept for sync+async multi-source, local+remote search. I'm going to stop working on it now and just use it as is for a few days to collect some observations. Besides the async behavior the main advantage, of course, is the reduction in bookkeeping since these are regular Consult sources, and simpler to write and plug into Here's a demo showing off all the sources, previews and the async updates (best watched full-screen): consult-web-demo-all.mp4
|
Beta Was this translation helpful? Give feedback.
-
|
A short update: Work on the prototype consult-web-mini continues. I've tried a few different web sources (sync and async), and it works well. Synchronous local sources work too, but async local sources via OS processes, like For dogfooding, sources I've added or am adding:
|
Beta Was this translation helpful? Give feedback.
-
|
I have been playing with @karthink's consult-web-mini, and I can now provide some more specific comments on this thread: Generally, I think the fact that we can do what @karthink has shown so far, suggests that it is possible to achieve the general idea here without changing anything in consult. So the question now is whether the convenience justifies adding this to consult or not. So far, my feeling is that it does not make sense to change the current Here are some specific observations/suggestions for more context:
(defun cw--multi-async (async sources)
"Merge the results of (a)sync SOURCES and pass it to function ASYNC."
(let ((candidates (make-vector (length sources) nil)))
(lambda (action)
(pcase action
((pred stringp)
(unless (equal action "")
(let ((idx 0))
(seq-doseq (src sources)
(let* ((face (and (plist-member src :face) `(face ,(plist-get src :face))))
(cat (plist-get src :category))
(items (plist-get src :items))
(narrow (plist-get src :narrow))
(type (or (car-safe narrow) narrow -1))
(pos idx))
(when (or (eq consult--narrow type)
(not (or consult--narrow (plist-get src :hidden))))
(condition-case nil
(progn
(when (functionp items)
(cond
((< (cdr (func-arity items)) 1)
(setq items (funcall items))
(aset candidates idx ; sync source, refresh now
(and items (cw--multi-propertize
items cat idx face)))
(funcall async 'flush)
(funcall async (apply #'append (append candidates nil))))
((< (cdr (func-arity items)) 2)
(setq items (funcall items action))
(aset candidates idx ; sync source, refresh now
(and items (cw--multi-propertize
items cat idx face)))
(funcall async 'flush)
(funcall async (apply #'append (append candidates nil))))
((< (cdr (func-arity items)) 3)
(funcall items action ; async source, refresh in callback
(lambda (response-items)
(when response-items
(aset candidates pos
(cw--multi-propertize response-items cat pos face))
(funcall async 'flush)
(funcall async (apply #'append (append candidates nil))))))))
))
(wrong-number-of-arguments
(message "%s got wrong number of arguments" items)
))))
(cl-incf idx)))))
(_ (funcall async action))))))
(defun cw--multi-group (sources cand transform)
"Return title of candidate CAND or TRANSFORM the candidate given SOURCES."
(if transform cand
(let* ((fun (and (plist-member (consult--multi-source sources cand) :group)
(plist-get (consult--multi-source sources cand) :group))))
(cond
((functionp fun)
(funcall fun sources cand transform))
((stringp fun)
fun)
((eq fun 'nil)
nil)
(t
(plist-get (consult--multi-source sources cand) :name))))))See more details on this one here: consult-web:#17
(defun demo-func (&optional no-action)
(interactive)
(let* ((selected (consult--read '("cand1" "cand2")
:prompt "Select: ")))
(unless no-action
(funcall #'print selected))
selected))This allows using the same source and consult machinery to get candidates but then bypass the return action and run some other functions on the candidate. For example a user can use the function above, but set (let ((cand (demo-func t)))
(message "%s was selected" cand))This is for example very useful in the
Sorry for the long post, and I may have more as I play more with @karthink's code. |
Beta Was this translation helpful? Give feedback.
-
I'd prefer a generalization of consult--multi instead of a separate API. The only advantage of a separate API would be that it could invent its own protocol (as in @karthink's prototype), but if possible a new distinct protocol is exactly something I'd like to avoid in favor of a general solution.
Maybe it is possible to wrap state functions such that a nil candidate is passed to the return action. But it may not be guaranteed that this effectively disables the state action completely.
I don't really see the difficulty. State functions usually consist of two parts (the preview and the action). You can only use the preview function and then combine it with arbitrary actions (user configurable if desired) to create a custom state function. |
Beta Was this translation helpful? Give feedback.
-
This part from Karthik's code looks similar to this code: Lines 2914 to 2926 in d84bbc1 Lines 2669 to 2676 in d84bbc1 Do you suggest we introduce some helper macros to make such code easier to write? |
Beta Was this translation helpful? Give feedback.
-
That's fine but then I would say that the consult--multi-* needs to be redesigned as well. For example, I just rewrote (defun consult-web--multi-predicate (sources cand)
"Predicate function called for each candidate CAND given SOURCES."
(let* ((src (consult--multi-source sources cand))
(narrow (plist-get src :narrow))
(type (or (car-safe narrow) narrow -1))
(pred (plist-get src :predicate))
(show t)
)
(if pred
(cond
((booleanp pred)
(setq show pred))
((and (functionp pred) (> (car (func-arity pred)) 0))
(setq show (funcall pred cand)))))
(and show
(or (eq consult--narrow type)
(not (or consult--narrow (plist-get src :hidden)))))))I have a feeling I am going to end up rewriting
Hmm!, let me try to explain this better. Let's say I want to make a custom function that is a wrapper around
Yes, I think in general this would be very useful to have helpers to make writing such code easier. Although I am not sure on the specifics so take this as food for thought. I think the composability would improve if there are helpers that allow this. |
Beta Was this translation helpful? Give feedback.
-
I see. However this sounds as if the generalization is not really a generalization. Then either the design cannot be generalized or the generalization is not the correct one.
Breaking :state into :preview and :return sounds like a reasonable approach. We could support this in consult--read and the sources. If both :preview and :return arguments are present, it would act like the current :state argument. However back then, when I introduced the :state function I believe I had such a design and then it turned out to work better if the state function has complete control over both steps, since sometimes a complete preview rollback isn't necessary when exiting. We have to check if this still applies.
I am not sure about this one. This is relatively trivial list combination code, basically the bread and butter of a lisp. Maybe it can be easier with quaisquotate and unquote/splicing. |
Beta Was this translation helpful? Give feedback.
-
But this should be fine. All we need in this case is a wrapper that builds the state function from :action, :preview etc. somewhat similar to what
I thought about this a bit more, and I realize I am not making the challenge clear here. So let me try to explain with examples (we can also get on a virtual call at some point if needed). (defvar consult--source-buffer-with-pred
`(:name "Buffer"
:narrow ?b
:category buffer
:face consult-buffer
:history buffer-name-history
:state ,#'consult--buffer-state
:default t
:items
,(lambda () (consult--buffer-query :sort 'visibility
:as #'buffer-name))
:predicate (lambda (cand) (string-match "\*.*\*" (substring-no-properties cand)))))If I now use consult--multi with just this one source, it ignores the :predicate! (the same is true for :lookup and etc.): (consult--multi (list consult--source-buffer-with-pred)) But if I deconstruct the plist and rewrite this with (consult--read (consult--buffer-query :sort 'visibility
:as #'buffer-name)
:prompt "Select: "
:predicate (lambda (cand) (string-match "\*.*\*" (substring-no-properties cand))))But if I make a plist that I want to pass to consult--multi, then it cannot easily be used with consult--read and vise versa. consult--read does not accept keywords like :action and :preview and consult--multi overrides :predicate and :lookup, ... IMHO a general Note that while in the simple example above, it is not that hard to deconstruct the plist etc., in a general case, where the user would want to be able to get a bunch of different sources and pass them to a generalized |
Beta Was this translation helpful? Give feedback.
-
Well, I don't agree with that as a goal. It is okay if the APIs are not 100% identical. They are already similar enough. The more consistent we can make the APIs, the better of course.
We should not go that way. The design right now is intentionally layered to reduce complexity. consult--read is supposed to be a more featureful completing-read wrapper, also with the goal of simplifying usage. consult--read still exposes all the completing-read details, and adds a few new features. The arguments like :predicate or :lookup which are not supported by consult--multi because they are technically not needed. :predicate is basically a kludge of completing-read and it is better to perform this filtering earlier when generating the candidates. :lookup is another kludge, only needed to resolve limitations around completion candidates with text properties. The problems you mention are not actually problems which hinder generalization. consult--read can stay a lower-level API, while consult--multi can stay on top, supporting single or multiple sources. The difficulty is generalizing consult--multi, with most (or all) of the major difficulties listed in #1007 (reply in thread). We can leave consult--read intact I believe. |
Beta Was this translation helpful? Give feedback.
-
|
I should maybe also mention that Consult doesn't have the "source concept" at its core. This is partially due to how the package grew and partially by design. Most Consult commands are supposed to operate on a single "source" (a set of candidates), in the style of completing-read and all the built-in Emacs commands. I believe the main usage difference is that with multi source commands you have to decide afterwards, while for single source commands, you have to decide up front which type of candidates you want. Coarse vs fine-grained commands. The current approach works very well for most use cases, but it hits limitations when you want to create an Omni search as in your package. Nevertheless it should be possible to generalize consult--multi to support such arbitrary combinations of sources. In any case, the closer the APIs are, the better. This certainly helps reuse. And furthermore, as was mentioned multiple times already, it will be good if the async protocol/pipeline can be reused as much as possible. Introducing another protocol is something which I would like to avoid. |
Beta Was this translation helpful? Give feedback.
-
|
Update: I have now implemented a fully working solution in consult-web's async branch. It does work for sync (like buffers), dynamic (like line-multi), and async (like grep or notmuch). However there are still areas where I simply took a "monkey see, monkey do approach" without fully understanding the consult flow. Can either of you @minad or @karthink take a look and see if there are improvements needed. Particularly here are some points to look at:
Finally, coming back to whether consult should generalize |
Beta Was this translation helpful? Give feedback.
-
|
Armin Darvish ***@***.***> writes:
Update:
I have now implemented a fully working solution in [consult-web's async
branch](https://github.com/armindarvish/consult-web/tree/async). It does work
for sync (like buffers), dynamic (like line-multi), and async (like grep or
notmuch). However there are still areas where I simply took a "monkey see,
monkey do approach" without fully understanding the consult flow. Can either of
you @minad or @karthink take a look and see if there are improvements needed.
Sounds great!
Particularly here are some points to look at:
1. I basically replace `consult--async-command` and `consult--dynamic-collection` into `consult-web--multi-dynamic-command`, the code here:
https://github.com/armindarvish/consult-web/blob/d7f68c24907137e3449c9173e06f6afbfb9c9168/consult-web.el#L1528C8-L1528C42
Then I combine `consult--async-process` and `consult--dynamic-compute` into a new generalized form in `consult-web--multi-dynamic-collection`:
https://github.com/armindarvish/consult-web/blob/d7f68c24907137e3449c9173e06f6afbfb9c9168/consult-web.el#L1497
Here, I am not sure if I am doing everything right. For example, in consult--dynamic-compute, there is a timer that reactivates in case of killed process:
https://github.com/minad/consult/blob/21d87b9f4066d662c18cc27e14636724c9f46b16/consult.el#L2444
However, I do not know if this is needed in my case and whether this is already achieved by the `consult--async-refresh-timer` inside `consult-web--multi-dynamic-command`o r not!:
https://github.com/armindarvish/consult-web/blob/d7f68c24907137e3449c9173e06f6afbfb9c9168/consult-web.el#L1533
I hope to merge back some of these features in a generalized form into
Consult. I am little bit short on time regarding Emacs hacking, given my
many projects. And this isn't a small undertaking as this long
discussion and your work on consult-web shows.
4. Perhaps related to 3 above is the debounce and throttle timing. I am not sure
if they are working as expected. IT feels that there is no delay between input
and sending the input to async/dynamic sources. But I am not sure how to go
about testing this?
The debouncing code is a bit brittle. I've also seen rare misbehavior
where the UI doesn't update. If you could give the debouncer a critical
look, I would very much appreciate it.
5. To add to our discussion on API, etc. as you can see I ended up demanding
that the type of the source (sync, v.s. dynamic, v.s. async) be specified as
opposed to trying to guess it or use `wrong-number-of-arguments` to catch this.
I think in the case of building a general omni-search tool, it's better that the
user specifies the type rather than us trying to guesss which one it
is.
I think both solutions are okay. Explicit might be better since it is
more clear for the reader. However auto-detection would also certainly
be robust enough.
Finally, coming back to whether consult should generalize `consult--multi`, I
can see two different approaches. One, we accept that it is possible to combine
sync and async sources as demonstrated in consult-web already and will point
other users to consult-web for example of implementation.
Well, it would be better to offer the necessary APIs in Consult. It is
not great if APIs have to be bent as much as you did.
Two, we port some of what I explained above about generalized
collection to consult, but personally based on the reinventing that I
had to do for different aspects of consult--multi, I think this has to
then be a new command (maybe called "consult--omni") otherwise I think
handling the collection and combining `consult--async-process` and
`consult--dynamic-compute` within the current `consult--multi`
approach can be tricky.
Yes, if it turns out that we cannot generalize consult--multi, then we
would have to go with a new command. But I am still convinced that a
generalization is possible.
|
Beta Was this translation helpful? Give feedback.
-
I can try, but so far I have not been able to find a good way to track and isolate these so I can test them. I believe
&
@minad I suggest when you find some time, you play with consult-web's async branch (note that I do not have any useful documentation for that yet, but I will add that some time soon). I think it would give you some ideas and food for thought for the generalization effort. For example, one thing I am completely bypassing is the command line arguments that gets passed with " -- --limit 100" to async commands because in multi-async scenario the same argument can mean very different things for different shell commands and generally it throws error becuase the command does not accept the argument or expects certain values etc. I don't know if there is a general solution to this; each async source (and or dynamic elisp sources) has to handle this inside their own builder/function. Also, I yet have to figure out how to do things like transforms of async sources without causing issues in other non-async sources, ... But that's just me. Obviously, you have a much better grasp of the consult inner details and the overall flow, and likely have thought about some of these before as well. So you will probably find much better elegant solutions to generalize everything. |
Beta Was this translation helpful? Give feedback.
-
|
By the way, one more thing that I forgot to mention and I have not been able to figure out is how to get the incoming new candidates in a multi-async read to get appended to the end of the list. Ideally I would want new groups to be added at the end of the current groups and new candidates to be added to the end of the group list, but don't know how to achieve this yet! Currently, the candidates get appended to a fixed position in the list. So if some async source candidates arrive later they will get appended to the middle of the list and that makes the UI expreience not ideal. I think the issue happens in but I do not understand the significance of this position argument and how it gets used in
|
Beta Was this translation helpful? Give feedback.
-
|
I've finally added support for asynchronous consult--multi sources. Asynchronous |
Beta Was this translation helpful? Give feedback.
-
|
@minad Thanks for letting us know. I think for consult-omni, I like the current implementation have I ended up with because it also adds some other features like dynamic grouping, etc. that is useful to have in consult-omni, but I think I can take some hints from your new By the way, speaking of dynamic grouping, I recently implemented a dynamic grouping in consult-gh (you can see a demo screenshot here: https://github.com/armindarvish/consult-gh?tab=readme-ov-file#dynamic-grouping-of-candidates) which works nicely despite being hacky. I think this can be another interesting idea for you to think about (just as food for thought) to introduce a syntax similar to async extra command line arguments but using elisp keywords (e.g. ":group", ":count", ":filter", ...). Of course this can also be an entirely new package separate from consult for grouping, filtering, etc. (perhaps after doing |
Beta Was this translation helpful? Give feedback.
-
|
Maybe you can rebase some of the functionality on top of the new machinery, which is relatively simple, but maybe also has a few rough edges and limitations. In any case I'd be interested in feedback and some limitations could be lifted over time. Your dynamic grouping is interesting. I also had the idea to implement a package similar to Marginalia which dynamically injects |
Beta Was this translation helpful? Give feedback.
-
|
@armindarvish I've pushed a breaking change. See ada079d for details. I hope it doesn't require too many adjustments on your side, since the async protocol stayed largely the same. |
Beta Was this translation helpful? Give feedback.
-
|
Thanks for letting me know @minad! I think I can adapt with some minimal changes. I was going to do that to take advantage of the new capabilities you have added anyway. So I can just adapt everything once I find some time. |
Beta Was this translation helpful? Give feedback.
-
|
@minad Any ideas on when you would be releasing the new breaking changes as stable? |
Beta Was this translation helpful? Give feedback.
-
|
I don't have plans yet. |
Beta Was this translation helpful? Give feedback.
-
Following up on our discussion on Reddit:
(CC @armindarvish)
Here's what I have right now, please ignore the
cw-prefix:cw--multi-asyncis intended to be used in a Consult async pipeline, and combines results from async or synchronoussources.cw--multi-propertizeis a helper function.Here is an example of using this in a
consult--multistyle function:There is no change to the definition of synchronous Consult sources. Async sources differ only in that their
:itemskey is a function accepting two arguments, the query and a callback. Here is an example of an async source (with:state,:previewetc omitted):And here is an example of usage with two more sources defined (one async, one sync):
consult-web-demo.mp4
I did not follow your latest comments on reddit, perhaps because I'm not fully familiar with
consult--readyet:Sorry for the trouble -- could you give me an example of what you're looking for?
Beta Was this translation helpful? Give feedback.
All reactions