From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Sergey Kostyaev Newsgroups: gmane.emacs.devel Subject: Re: Add elisa to GNU ELPA Date: Tue, 16 Jul 2024 20:57:41 +0700 Message-ID: References: <8734o9sdig.fsf@posteo.net> Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3774.600.62\)) Content-Type: multipart/alternative; boundary="Apple-Mail=_3BADC001-8431-46DC-A8AE-3C09F1435D7F" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="18427"; mail-complaints-to="usenet@ciao.gmane.io" Cc: emacs-devel@gnu.org To: Philip Kaludercic Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Tue Jul 16 16:11:49 2024 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1sTiuD-0004Oq-47 for ged-emacs-devel@m.gmane-mx.org; Tue, 16 Jul 2024 16:11:49 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1sTitH-0006Pa-AU; Tue, 16 Jul 2024 10:10:51 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1sTigt-0000l1-9N for emacs-devel@gnu.org; Tue, 16 Jul 2024 09:58:03 -0400 Original-Received: from mail-lf1-x12f.google.com ([2a00:1450:4864:20::12f]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1sTign-0001s8-Ls for emacs-devel@gnu.org; Tue, 16 Jul 2024 09:58:03 -0400 Original-Received: by mail-lf1-x12f.google.com with SMTP id 2adb3069b0e04-52eafec1e84so7744542e87.0 for ; Tue, 16 Jul 2024 06:57:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1721138275; x=1721743075; darn=gnu.org; h=references:to:cc:in-reply-to:date:subject:mime-version:message-id :from:from:to:cc:subject:date:message-id:reply-to; bh=zvY1Vk9MU650/TV1ky3cDwVgwTtHxwxISi8mTkDQYUI=; b=TQnV06N4GCl3RYa2qUlYNU7ordh4KDO8tIDkuIrjq6u4GgzZ/NJaB7ECWCfYn0Th5T M2LLkmxT1asoTlGf3AHi6YxUfyIpBATUyFIQHKAqpebBgY+rKsVjy2bhvbt3ZITjjWhI 4a11advPrNgqZeaIsk/Jwlc2DE2oSddkSkJIkE+4darfVfmp6F3uCyPZQEj4ser+mTa2 lVAsrHl3dpnH8SeQE76p+XEuSRqrdMgLLvg2EwRbrDaKmy70Ta4Cndc0XFT6pqe+AWnD x4cQp4eNvJIAamBD+1ED072sPiO7WFD8LaiXm7CssWbRgAULhQAoH/QpOxwn5DdTFys0 DzZA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1721138275; x=1721743075; h=references:to:cc:in-reply-to:date:subject:mime-version:message-id :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=zvY1Vk9MU650/TV1ky3cDwVgwTtHxwxISi8mTkDQYUI=; b=Eq2k/f/RbCOibWCwAPpUfwRcyK10BvKPXCgOK2vnS9EC6q8OnK7i2lHFzscUvnm0O2 +v6tXF+BFE/a98ln3oU3VK1bGu4xzry80WPv6cCO9I2WpF0kUBgW6mSMg3DC+j7rHyel 2WDTpHA72+fMDn5sbm40anhhEF491uphFckVUKaRDtX75IbWEJI7U9wU8nmtsgzWoJ6f g/kswWa0vtcmNEoZuhq4AACATFXNqCTRkajQEMCGBAHgDog14Ou2igcTYXQu1+r1Ry0g 5uUv0t1viV89cnK49jfJQdpRK1cQVi77jk9sdRnDe0y4k0/r/afSq1d5C8xU9c+2i+Vu JYtQ== X-Gm-Message-State: AOJu0YwZe3jBpmzVeQ62TVx90+w06Gie7o8rfSZgBvY0hBNIlEM36Eka WWYUtlme4rFUS0uYMdSaXy9WxIfs7ECmY3+QB2F5xNbl44F/9P30 X-Google-Smtp-Source: AGHT+IH536towTjtfgQPatCEjDQ7kNr+QluiblCFWKXBkvevKocrb9QwkNZF6Tase00F+kx6VOhoUA== X-Received: by 2002:a05:6512:3b06:b0:52c:8b03:99d6 with SMTP id 2adb3069b0e04-52edef1cc47mr1525849e87.6.1721138273901; Tue, 16 Jul 2024 06:57:53 -0700 (PDT) Original-Received: from smtpclient.apple ([90.189.160.165]) by smtp.gmail.com with ESMTPSA id 2adb3069b0e04-52ed25398d2sm1150918e87.287.2024.07.16.06.57.52 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Tue, 16 Jul 2024 06:57:53 -0700 (PDT) In-Reply-To: <8734o9sdig.fsf@posteo.net> X-Mailer: Apple Mail (2.3774.600.62) Received-SPF: pass client-ip=2a00:1450:4864:20::12f; envelope-from=sskostyaev@gmail.com; helo=mail-lf1-x12f.google.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-Mailman-Approved-At: Tue, 16 Jul 2024 10:10:49 -0400 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.devel:321706 Archived-At: --Apple-Mail=_3BADC001-8431-46DC-A8AE-3C09F1435D7F Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Thank you for review. > 16 =D0=B8=D1=8E=D0=BB=D1=8F 2024=E2=80=AF=D0=B3., =D0=B2 19:54, Philip = Kaludercic =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB= (=D0=B0): >=20 > Despite > reading through the entire source code, I have no idea what you are > trying to do with this package. It would be nice to have some more > context in the Commentary section and elaborate a number of = user-facing > docstrings, unless this is intended to be expert software, for people > familiar with whatever the field is. I need to improve it. With README.org it should be = cleaner, but It should be understandable without it. Why should we remove elisa customization group? I think there are many = customization options to move it to group. > @@ -48,68 +47,61 @@ > (make-llm-ollama > :embedding-model = "nomic-embed-text")) > "Embeddings provider to generate embeddings." > - :group 'elisa > - :type '(sexp :validate 'cl-struct-p)) > + :type '(sexp :validate 'cl-struct-p)) ;a more specific predicate = here? It should be `llm` provider. Unfortunately there are no more specific = predicate for that. > (defcustom elisa-db-directory (file-truename > (file-name-concat > user-emacs-directory "elisa")) > "Directory for elisa database." > - :group 'elisa > - :type 'directory) > + :type 'directory) ;is it necessary that it = exists? If you mean option - then yes. If you mean directory, I can create it in = `elisa' package. > + (regexp-opt (mapcar #'car reps)) ;is the last one really \0 or = \\0? Really. Probably impossible case, but it=E2=80=99s defensive = programming. > +;; a number of these functions seem like something that should be = added to the core of Emacs or at least a common ELPA package... > (defun elisa-dot-product (v1 v2) > "Calculate the dot produce of vectors V1 and V2." Good idea. How can we implement it? > - (format "%s -f html --to plain" > - (executable-find elisa-pandoc-executable)) > + (format "%s --from html --to plain" elisa-pandoc-executable) > buffer-name t) > buffer-name))) We should keep executable-find call here. It was feature request from = users. It can be useful with per-directory PATH, like with direnv. >=20 > (defun elisa-fts-query (prompt) > "Return fts match query for PROMPT." > - (thread-last > + (thread-last ;i belive you can do all of = this with a single regular expression... > prompt > (string-trim) > (downcase) Maybe. But I prefer to keep it readable. > @@ -1071,7 +1044,7 @@ WHERE d.rowid in %s;" > (when-let ((kind (cl-first row)) > (path (cl-second row)) > (text (cl-third row))) > - (pcase kind > + (pcase kind ;is this a `pcase-exhaustive'? > ("web" > (ellama-context-add-webpage-quote-noninteractive = path path text)) > ("file" I think it should be extendable, but for now it is not. Best regards, Sergey Kostyaev --Apple-Mail=_3BADC001-8431-46DC-A8AE-3C09F1435D7F Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8
Thank you = for review.

16 =D0=B8=D1=8E=D0=BB=D1=8F = 2024=E2=80=AF=D0=B3., =D0=B2 19:54, Philip Kaludercic = <philipk@posteo.net> = =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0):

Des= pite
reading through the entire source code, I have no idea what you = are
trying to do with this package.  It would be nice to have = some more
context in the Commentary section and elaborate a number of = user-facing
docstrings, unless this is intended to be expert = software, for people
familiar with whatever the field = is.

I need to improve = it. With README.org it should be = cleaner, but It should be understandable without = it.

Why should we remove elisa customization = group? I think there are many customization options to move it to = group.

@@ -48,68 +47,61 = @@
= = = = =    (make-llm-ollama
=     :embedding-model "nomic-embed-text"))
=   "Embeddings provider to generate embeddings."
- =  :group 'elisa
-  :type '(sexp :validate = 'cl-struct-p))
+  :type '(sexp :validate 'cl-struct-p)) ;a more = specific predicate here?

It = should be `llm` provider. Unfortunately there are no more specific = predicate for that.

= (defcustom elisa-db-directory (file-truename
=       (file-name-concat
= user-emacs-directory "elisa"))
  "Directory for = elisa database."
-  :group 'elisa
-  :type = 'directory)
+  :type 'directory) =             &n= bsp;       ;is it necessary that it = exists?

If you mean option - = then yes. If you mean directory, I can create it in `elisa' = package.

+ =     (regexp-opt (mapcar #'car reps)) ;is the last = one really \0 or \\0?

Really. = Probably impossible case, but it=E2=80=99s defensive = programming.

+;; a number of these functions seem like = something that should be added to the core of Emacs or at least a common = ELPA package...
(defun elisa-dot-product (v1 v2)
=   "Calculate the dot produce of vectors V1 and = V2."

Good idea. How can we = implement it?

-       (format = "%s -f html --to plain"
- =       (executable-find = elisa-pandoc-executable))
+ =       (format "%s --from html --to plain" = elisa-pandoc-executable)
=        buffer-name t)
=       buffer-name)))

We should keep executable-find call here. It = was feature request from users. It can be useful with per-directory = PATH, like with direnv.


= (defun elisa-fts-query (prompt)
  "Return fts match query = for PROMPT."
-  (thread-last
+  (thread-last =             &n= bsp;           &nbs= p;;i belive you can do all of this with a single regular = expression...
    prompt
=     (string-trim)
=     (downcase)

Maybe. But I prefer to keep it readable.
@@ -1071,7 +1044,7 @@ WHERE d.rowid in %s;"
= = =   (when-let ((kind (cl-first row))
=      (path (cl-second row))
=      (text (cl-third row)))
- =     (pcase kind
+     (pcase = kind        ;is this a = `pcase-exhaustive'?
=       ("web"
= (ellama-context-add-webpage-quote-noninteractive path path = text))
= = =       ("file"

I think it should be extendable, but for now it is = not.

Best regards,
Sergey = Kostyaev

= --Apple-Mail=_3BADC001-8431-46DC-A8AE-3C09F1435D7F--