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

Add reazon-run-with-config and 'timeout' option #12

Merged
merged 1 commit into from
Aug 14, 2021

Conversation

ethan-leba
Copy link
Contributor

Hi again Nick,

As the title states this PR introduces a new run endpoint, reazon-run-with-config, and the current only config option is :timeout, which specifies what the maximum time a query can run for.

An :check-occurs config option could also be added later on to disable that feature for more performance.

My personal usecase for this is running reazon queries with an infinite search space interactively, where it either quickly succeeds or runs forever. Without this modification, you'd have to C-g out of every failed search.

Let me know if this looks OK!

@nickdrozd
Copy link
Owner

That's a cool idea. I'll take a look.

What sorts of queries are you running?

reazon.el Outdated
@@ -40,6 +40,8 @@

;;; Code:

(define-error 'reazon-timeout-error "The reazon query has exceeded it's time limit")
Copy link
Owner

@nickdrozd nickdrozd Aug 13, 2021

Choose a reason for hiding this comment

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

Suggested change
(define-error 'reazon-timeout-error "The reazon query has exceeded it's time limit")
(define-error 'reazon-timeout
"The reazon query has exceeded its time limit")

reazon.el Outdated
Comment on lines 186 to 192
(result (list (car stream))))
(ignore-error 'reazon-timeout-error
(let ((rest (reazon--pull (cdr stream) stop-time)))
(while (and rest (not (zerop count)))
(setq count (1- count))
(setq result (cons (car rest) result))
(setq rest (reazon--pull (cdr rest) stop-time)))))
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
(result (list (car stream))))
(ignore-error 'reazon-timeout-error
(let ((rest (reazon--pull (cdr stream) stop-time)))
(while (and rest (not (zerop count)))
(setq count (1- count))
(setq result (cons (car rest) result))
(setq rest (reazon--pull (cdr rest) stop-time)))))
(let ((count (if n (1- n) -1))
(result (list (car stream)))
(rest (ignore-error 'reazon-timeout-error
(reazon--pull (cdr stream) stop-time))))
(while (and rest (not (zerop count)))
(setq count (1- count))
(setq result (cons (car rest) result))
(setq rest (ignore-error 'reazon-timeout-error
(reazon--pull (cdr rest) stop-time))))
(nreverse result))))

Copy link
Owner

@nickdrozd nickdrozd Aug 13, 2021

Choose a reason for hiding this comment

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

This is written as a while loop, but it's really a do-while loop. Does Elisp have that? Otherwise there's this ugly duplication. (This is a pre-existing problem.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i don't think so unfortunately, but the new exception-less impl. doesn't have duplication anymore

reazon.el Outdated
(let ((result stream))
(while (functionp result)
(when (and stop-time (< stop-time (float-time)))
Copy link
Owner

Choose a reason for hiding this comment

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

Does this really need to throw an error? If the timeout is reached, then it will just return whatever it's gotten so far, right? Maybe it could just break the while loop here and return.

On the other hand, the user might want to know if their query has stopped because it reached the end or because it hit a timeout and there was still more to do.

reazon.el Outdated
Comment on lines 408 to 422
(declare (indent 2))
(if (listp var/list)
(let ((vars var/list)
(q (gensym)))
`(reazon-run-with-config ,n ,q ,config
(reazon-fresh ,vars
(reazon-== (list ,@vars) ,q)
,@goals)))
(let ((var var/list))
`(let ((,var (reazon--make-variable ',var))
(stop-time (and (plist-get ,config :timeout)
(+ (plist-get ,config :timeout) (float-time)))))
(mapcar
(reazon--reify ,var)
(reazon--take ,n (reazon--run-goal (reazon-conj ,@goals) stop-time) stop-time))))))
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
(declare (indent 2))
(if (listp var/list)
(let ((vars var/list)
(q (gensym)))
`(reazon-run-with-config ,n ,q ,config
(reazon-fresh ,vars
(reazon-== (list ,@vars) ,q)
,@goals)))
(let ((var var/list))
`(let ((,var (reazon--make-variable ',var))
(stop-time (and (plist-get ,config :timeout)
(+ (plist-get ,config :timeout) (float-time)))))
(mapcar
(reazon--reify ,var)
(reazon--take ,n (reazon--run-goal (reazon-conj ,@goals) stop-time) stop-time))))))
(declare (indent 3))
(if (listp var/list)
(let ((vars var/list)
(q (gensym)))
`(reazon-run-with-config ,n ,q ,config
(reazon-fresh ,vars
(reazon-== (list ,@vars) ,q)
,@goals)))
(let ((var var/list))
`(let ((,var (reazon--make-variable ',var))
(stop-time (and (plist-get ,config :timeout)
(+ (plist-get ,config :timeout) (float-time)))))
(mapcar
(reazon--reify ,var)
(reazon--take ,n (reazon--run-goal (reazon-conj ,@goals) stop-time) stop-time))))))

reazon.el Outdated
,@goals)))
(let ((var var/list))
`(let ((,var (reazon--make-variable ',var))
(stop-time (and (plist-get ,config :timeout)
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a way to do the plist-get just once?

@nickdrozd
Copy link
Owner

An :check-occurs config option could also be added later on to disable that feature for more performance.

#6, for reference.

@nickdrozd
Copy link
Owner

@ebpa, what do you think?

Comment on lines 58 to 61
(ert-deftest reazon-test-interface-run-with-config ()
(reazon--should-equal '()
(reazon-run-with-config nil q '(:timeout 0.01) (reazon-test--nevero)))
(should (consp (reazon-run-with-config nil q '(:timeout 0.01) (reazon-test--alwayso)))))
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
(ert-deftest reazon-test-interface-run-with-config ()
(reazon--should-equal '()
(reazon-run-with-config nil q '(:timeout 0.01) (reazon-test--nevero)))
(should (consp (reazon-run-with-config nil q '(:timeout 0.01) (reazon-test--alwayso)))))
(ert-deftest reazon-test-interface-run-with-config ()
(reazon--should-equal '()
(reazon-run-with-config nil q '(:timeout 0.01)
(reazon-test--nevero)))
(should
(consp
(reazon-run-with-config nil q '(:timeout 0.01)
(reazon-test--alwayso)))))

Copy link
Owner

Choose a reason for hiding this comment

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

Can we do any better than consp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched this to a length >3 check, abritrarily chose 3

@ethan-leba
Copy link
Contributor Author

Removed the exception handling, and went for a different (and i think more idiomatic) approach for how the configuration gets passed in. Inspired by json-read, it expects users to let-bind configuration options. This'll also be helpful for toggling occurs, as explicitly passing that parameter around would be a nightmare!

@ethan-leba
Copy link
Contributor Author

That's a cool idea. I'll take a look.

What sorts of queries are you running?

I'm using reazon as a code generation engine for a structural editing plugin -- so the queries are asking for a list of tokens that parse for a given grammar, along with some extra criteria.

@nickdrozd
Copy link
Owner

I'm using reazon as a code generation engine for a structural editing plugin -- so the queries are asking for a list of tokens that parse for a given grammar, along with some extra criteria.

If you have that up somewhere I would love to take a look. I've mostly used Reazon for logic puzzles and generating instances of stuff (for instance, https://nickdrozd.github.io/2018/08/14/modal-sentences.html).

Copy link
Owner

@nickdrozd nickdrozd left a comment

Choose a reason for hiding this comment

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

Agreed that this approach is more idiomatic, and it's simpler too. Thanks!

@nickdrozd nickdrozd merged commit 78dffda into nickdrozd:master Aug 14, 2021
@ethan-leba
Copy link
Contributor Author

It's still very much a WIP, but I just put the repo on GH -- here's a link to the reazon section: https://github.com/ethan-leba/tree-edit/blob/main/tree-edit.el#L560

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