alda-lang / alda-server-clj

A Clojure implementation of an Alda server
Other
2 stars 2 forks source link

Add basic support for history option in the alda server #4

Closed jgkamat closed 7 years ago

jgkamat commented 7 years ago

This is my first attempt to add support for a history flag (server side). (STILL IN DEVELOPMENT)

blocking alda-lang/alda-client-java#1

(I don't know clojure too well, so this might be terrible, let me know if it is :P)

Right now the following works: alda play --history '(tempo! 500)' --play 'piano: c d e'

But this does not work: alda play --history 'piano: c d e f g' --play 'c d e'

The error that I get is:

c d e
^
Expected one of:
"⚙"
#"[a-zA-Z]{2}[\w_]*"

which I'd assume means the syntax checking regexes need to be changed, which I'm not sure how to do (and I don't want to get wrong, since I don't know 100% of the alda syntax). I might take a stab at that tomorrow/later this week.

One of the thinigs I'm pretty worried about is error checking (I'm not sure if I got it right, could you take a look at that for me?).

Let me know if theres anything overall I can improve too! :smile:

jgkamat commented 7 years ago

This regex seems to be located here. I'm not sure how to modify the alda-server-clj boot file to point to my local repo though, (I think such instructions should be added to the alda root CONTRIBUTING file, or maybe the alda-core CONTRIBUTING maybe?).

I'm also not sure if we can just make the regex more lax, or if we should add a new regex for the body string (and use the old one for history).

jgkamat commented 7 years ago

@daveyarwood I think I just fixed the piano: issue in the last commit. Should the parse-*-with-context function be used for the history too? alda play --history 'c d e' --code 'piano: c d e' fails with the same error as above, but I'm not sure if that's the behavior we want or not (since this was being used for --code before. Let me know and I'll swap the parsing of the history over to this other method as well!

Let me know if the error checking is off, I think I got it, but I'm not sure if = is the proper way to check if the keyword is an error keyword.

jgkamat commented 7 years ago

I think that parsing the history is needed for consistent behavior with the old repl, but I'm not sure how to do it. The parse-to-map-with-context function seems to return a slightly different result (which results in no notes playing). I think the continue function can't handle the difference for some reason. Do you have any idea what this could be?

This is an example of the current map output:

{:chord-mode false, :current-instruments #{"piano-IKZfn"}, :beats-tally nil, :instruments {"piano-IKZfn" {:octave 4, :current-offset #alda.lisp.model.records.AbsoluteOffset{:offset 2500.0}, :key-signature {}, :config {:type :midi, :patch 1}, :duration {:beats 1, :ms nil}, :min-duration nil, :volume 1.0, :last-offset #alda.lisp.model.records.AbsoluteOffset{:offset 2000.0}, :id "piano-IKZfn", :quantization 0.9, :duration-inside-cram nil, :tempo 120, :panning 0.5, :current-marker :start, :time-scaling 1, :stock "midi-acoustic-grand-piano", :track-volume 0.7874015748031497}}, :markers {:start 0}, :cram-level 0, :global-attributes {}, :nicknames {}, :beats-tally-default nil}

And this is an example of the output from parse-to-map-with-context

{:event-type :part, :instrument-call {:names ["piano"]}, :events ({:event-type :note, :pitch-fn #object[clojure.lang.AFunction$1 0x787667b3 "clojure.lang.AFunction$1@787667b3"], :beats nil, :ms nil, :slur? nil} {:event-type :note, :pitch-fn #object[clojure.lang.AFunction$1 0x4201b534 "clojure.lang.AFunction$1@4201b534"], :beats nil, :ms nil, :slur? nil} {:event-type :note, :pitch-fn #object[clojure.lang.AFunction$1 0x1a7208a "clojure.lang.AFunction$1@1a7208a"], :beats nil, :ms nil, :slur? nil} {:event-type :note, :pitch-fn #object[clojure.lang.AFunction$1 0x60a67757 "clojure.lang.AFunction$1@60a67757"], :beats nil, :ms nil, :slur? nil} {:event-type :note, :pitch-fn #object[clojure.lang.AFunction$1 0x2b26df0f "clojure.lang.AFunction$1@2b26df0f"], :beats nil, :ms nil, :slur? nil})}

Do you have any idea what could be going wrong? Do I need to use a different to map function?

These are the changes I made

daveyarwood commented 7 years ago

Let me know if the error checking is off, I think I got it, but I'm not sure if = is the proper way to check if the keyword is an error keyword.

Yep! Compared to Emacs Lisp and Common Lisp, Clojure has very simple equality semantics -- there is just a single function, = for comparing all kinds of things.

@daveyarwood I think I just fixed the piano: issue in the last commit. Should the parse-*-with-context function be used for the history too? alda play --history 'c d e' --code 'piano: c d e' fails with the same error as above, but I'm not sure if that's the behavior we want or not (since this was being used for --code before. Let me know and I'll swap the parsing of the history over to this other method as well!

Actually, I think the way you have it is the right way. The history part should always be a valid score in its entirety, so parse-input should give us the correct behavior there.

I captured a quick Clojure REPL session that might help: https://gist.github.com/daveyarwood/9ea88d359252efb086c80839c31b6769

tl;dr:


; returns eval-able Clojure code
alda.parser-util=> (def history (parse-input "piano: c d e"))
; returns the score data map
alda.parser-util=> (def history (eval history))

; returns a tuple of :music data and a list of events (f, g, a, b, octave-up, c)
alda.parser-util=> (def more-input (parse-to-events-with-context "f g a b > c"))
; we just want the list of events
alda.parser-util=> (def more-input (second more-input))

; continue takes a variable number of arguments -- we want to unroll the list of events into individual arguments, so we use apply
alda.parser-util=> (apply continue history more-input)
jgkamat commented 7 years ago

Hmm, thats interesting. It makes sense that 'history' should be a valid score, but in the case of the repl, if the user enters c d e (I think this was part of the tutorial), and stores it to history for the next command, it would fail, since c d e isn't a valid score. I'm not sure how to resolve this. We could:

  1. Default to piano in the repl, since that is what people usually want anyway
  2. Detect history errors, and if there is one, clear the last entry of history, and resubmit the repl request without it.
  3. Make the server accept more lenient history
  4. Ignore history errors in the repl. (This might be very bad)

Let me know if you know any way to resolve this...

Also, how did you get the clojure repl? Is there a boot command to give me a repl on the server? :smile:

daveyarwood commented 7 years ago

Ah, that's a good point.

  1. Detect history errors, and if there is one, clear the last entry of history, and resubmit the repl request without it.

I think this is a decent workaround, although it isn't perfect. I can't really think of any other way to do it at the moment that wouldn't involve doing an extra parsing pass to validate that "history + new input" is a valid score. One downside to doing it this way is that history errors will not be caught until the user submits the next line of code at the REPL, so it's not ideal. Maybe we can roll with the workaround for now and come back to this problem later?

Also, how did you get the clojure repl? Is there a boot command to give me a repl on the server? :smile:

If you have Boot installed and you're in the top level folder of this repo, you can run:

$ boot repl

which will get you into a REPL in the boot.user namespace. From there, you can require and use any namespace available in the project, like alda.parser, alda.lisp, etc.

Oftentimes I already have an idea of what namespace I want to start in, so I use the -n option when I start the REPL:

$ boot repl -n alda.parser-util

which saves me a require or in-ns.

Another handy thing: if you make changes to a file and you want to test them out, you can reload that namespace without restarting the REPL:

; after making changes to parser.clj
alda.parser=> (require '[alda.parser] :reload)
jgkamat commented 7 years ago

@daveyarwood I found another small issue with the last version, it appeared as if the offsets from the history block were being applied to the code block (so alda play --history 'piano: c d e f g' --code "c d e" would not play immediately).

I fixed this by clearing the offsets in the history block, but I think the solution I made is kind of unpleasant. I'm not sure if there's a better way to do this (maybe passing an option to continue?).

daveyarwood commented 7 years ago

OK, I think I can solve both of these problems!

Validating scores

I've been doing a lot of thinking about what should or should not be considered "valid input" by the parser. It occurred to me that maybe the parser should not be responsible for validating that notes/chords/etc. may not appear in a score before the first part (e.g. piano:) is created.

We want to be able to do these "partial parses" of small parts of a score (e.g. single lines of input in the Alda REPL), so from the parser's perspective, it would be simpler if it accepted any valid input, and didn't have to worry about where in the score the input might be.

I think it would be better if we made it the responsibility of the score evaluation process to throw errors if events are provided that don't make sense, like trying to play notes when there aren't any instruments yet.

Currently, score evaluation will just ignore any notes that occur before a part is created:

alda.lisp=> (def s (score))
#'alda.lisp/s

; a new, empty score
alda.lisp=> s
{:chord-mode false, :current-instruments #{}, :voice-instruments nil, :events #{}, :beats-tally nil, :instruments {}, :markers {:start 0}, :cram-level 0, :global-attributes {}, :current-voice nil, :nicknames {}, :beats-tally-default nil}

; if we try to add notes, there is no change
alda.lisp=> (continue s (note (pitch :c)) (note (pitch :d)))
{:chord-mode false, :current-instruments #{}, :voice-instruments nil, :events #{}, :beats-tally nil, :instruments {}, :markers {:start 0}, :cram-level 0, :global-attributes {}, :current-voice nil, :nicknames {}, :beats-tally-default nil}

; once we add a part, notes can be added to the score
alda.lisp=> (continue s (note (pitch :c)) (note (pitch :d)) (part "piano") (note (pitch :e)))
{:chord-mode false, :current-instruments #{"piano-w5baf"}, :events #{#alda.lisp.model.records.Note{:offset 0, :instrument "piano-w5baf", :volume 1.0, :track-volume 0.7874015748031497, :panning 0.5, :midi-note 64, :pitch 329.6275569128699, :duration 450.0, :voice nil}}, :beats-tally nil, :instruments {"piano-w5baf" {:octave 4, :current-offset #alda.lisp.model.records.AbsoluteOffset{:offset 500.0}, :key-signature {}, :config {:type :midi, :patch 1}, :duration {:beats 1, :ms nil}, :min-duration nil, :volume 1.0, :last-offset #alda.lisp.model.records.AbsoluteOffset{:offset 0}, :id "piano-w5baf", :quantization 0.9, :duration-inside-cram nil, :tempo 120, :panning 0.5, :current-marker :start, :time-scaling 1, :stock "midi-acoustic-grand-piano", :track-volume 0.7874015748031497}}, :markers {:start 0}, :cram-level 0, :global-attributes {}, :nicknames {}, :beats-tally-default nil}

; pretty printing the above for readibility
alda.lisp=> (pp)
{:chord-mode false,
 :current-instruments #{"piano-w5baf"},
 :events
 #{{:offset 0,
    :instrument "piano-w5baf",
    :volume 1.0,
    :track-volume 0.7874015748031497,
    :panning 0.5,
    :midi-note 64,
    :pitch 329.6275569128699,
    :duration 450.0,
    :voice nil}},
 :beats-tally nil,
 :instruments
 {"piano-w5baf"
  {:octave 4,
   :current-offset {:offset 500.0},
   :key-signature {},
   :config {:type :midi, :patch 1},
   :duration {:beats 1, :ms nil},
   :min-duration nil,
   :volume 1.0,
   :last-offset {:offset 0},
   :id "piano-w5baf",
   :quantization 0.9,
   :duration-inside-cram nil,
   :tempo 120,
   :panning 0.5,
   :current-marker :start,
   :time-scaling 1,
   :stock "midi-acoustic-grand-piano",
   :track-volume 0.7874015748031497}},
 :markers {:start 0},
 :cram-level 0,
 :global-attributes {},
 :nicknames {},
 :beats-tally-default nil}
nil
alda.lisp=>

So I think for now, we should just parse both the history and the new input as events, and use both event sequences to continue a new score, i.e. something like this:

alda.lisp=> (require '[alda.parser-util :as p])
nil

alda.lisp=> (def history (-> (p/parse-to-events-with-context "piano: c d e") second))
#'alda.lisp/history

; note: this is considered a single "part" event containing 3 notes
alda.lisp=> history
{:event-type :part, :instrument-call {:names ["piano"]}, :events ({:event-type :note, :pitch-fn #object[clojure.lang.AFunction$1 0x71c3c7e8 "clojure.lang.AFunction$1@71c3c7e8"], :beats nil, :ms nil, :slur? nil} {:event-type :note, :pitch-fn #object[clojure.lang.AFunction$1 0x242a0167 "clojure.lang.AFunction$1@242a0167"], :beats nil, :ms nil, :slur? nil} {:event-type :note, :pitch-fn #object[clojure.lang.AFunction$1 0x54151611 "clojure.lang.AFunction$1@54151611"], :beats nil, :ms nil, :slur? nil})}

alda.lisp=> (def new-input (-> (p/parse-to-events-with-context "f g a b > c") second))
#'alda.lisp/new-input

; seq of events: f, g, a, b, >, c
alda.lisp=> new-input
({:event-type :note, :pitch-fn #object[clojure.lang.AFunction$1 0x344dd5b6 "clojure.lang.AFunction$1@344dd5b6"], :beats nil, :ms nil, :slur? nil} {:event-type :note, :pitch-fn #object[clojure.lang.AFunction$1 0x404be024 "clojure.lang.AFunction$1@404be024"], :beats nil, :ms nil, :slur? nil} {:event-type :note, :pitch-fn #object[clojure.lang.AFunction$1 0x1befeb74 "clojure.lang.AFunction$1@1befeb74"], :beats nil, :ms nil, :slur? nil} {:event-type :note, :pitch-fn #object[clojure.lang.AFunction$1 0x9a3e790 "clojure.lang.AFunction$1@9a3e790"], :beats nil, :ms nil, :slur? nil} {:event-type :attribute-change, :attr :octave, :val :up} {:event-type :note, :pitch-fn #object[clojure.lang.AFunction$1 0x53196377 "clojure.lang.AFunction$1@53196377"], :beats nil, :ms nil, :slur? nil})

; start with a new score, add the history events, then add the events from new input
alda.lisp=> (-> (score) (continue history) (continue new-input))
{:chord-mode false, :current-instruments #{"piano-qoOmj"}, :events #{#alda.lisp.model.records.Note{:offset 0, :instrument "piano-qoOmj", :volume 1.0, :track-volume 0.7874015748031497, :panning 0.5, :midi-note 60, :pitch 261.6255653005986, :duration 450.0, :voice nil} #alda.lisp.model.records.Note{:offset 1000.0, :instrument "piano-qoOmj", :volume 1.0, :track-volume 0.7874015748031497, :panning 0.5, :midi-note 64, :pitch 329.6275569128699, :duration 450.0, :voice nil} #alda.lisp.model.records.Note{:offset 3500.0, :instrument "piano-qoOmj", :volume 1.0, :track-volume 0.7874015748031497, :panning 0.5, :midi-note 72, :pitch 523.2511306011972, :duration 450.0, :voice nil} #alda.lisp.model.records.Note{:offset 3000.0, :instrument "piano-qoOmj", :volume 1.0, :track-volume 0.7874015748031497, :panning 0.5, :midi-note 71, :pitch 493.8833012561241, :duration 450.0, :voice nil} #alda.lisp.model.records.Note{:offset 500.0, :instrument "piano-qoOmj", :volume 1.0, :track-volume 0.7874015748031497, :panning 0.5, :midi-note 62, :pitch 293.6647679174076, :duration 450.0, :voice nil} #alda.lisp.model.records.Note{:offset 2000.0, :instrument "piano-qoOmj", :volume 1.0, :track-volume 0.7874015748031497, :panning 0.5, :midi-note 67, :pitch 391.99543598174927, :duration 450.0, :voice nil} #alda.lisp.model.records.Note{:offset 2500.0, :instrument "piano-qoOmj", :volume 1.0, :track-volume 0.7874015748031497, :panning 0.5, :midi-note 69, :pitch 440.0, :duration 450.0, :voice nil} #alda.lisp.model.records.Note{:offset 1500.0, :instrument "piano-qoOmj", :volume 1.0, :track-volume 0.7874015748031497, :panning 0.5, :midi-note 65, :pitch 349.2282314330039, :duration 450.0, :voice nil}}, :beats-tally nil, :instruments {"piano-qoOmj" {:octave 5, :current-offset #alda.lisp.model.records.AbsoluteOffset{:offset 4000.0}, :key-signature {}, :config {:type :midi, :patch 1}, :duration {:beats 1, :ms nil}, :min-duration nil, :volume 1.0, :last-offset #alda.lisp.model.records.AbsoluteOffset{:offset 3500.0}, :id "piano-qoOmj", :quantization 0.9, :duration-inside-cram nil, :tempo 120, :panning 0.5, :current-marker :start, :time-scaling 1, :stock "midi-acoustic-grand-piano", :track-volume 0.7874015748031497}}, :markers {:start 0}, :cram-level 0, :global-attributes {}, :nicknames {}, :beats-tally-default nil}

; to play the whole score, we just feed it to alda.now/play-score!
alda.lisp=> (require '[alda.now :as now])
nil
alda.lisp=> (now/play-score! (-> (score) (continue history) (continue new-input)))
Jan 31, 2017 7:33:59 AM com.jsyn.engine.SynthesisEngine start
INFO: Pure Java JSyn from www.softsynth.com, rate = 44100, RT, V16.7.3 (build 457, 2014-12-25)
#object[alda.sound$play_BANG_$fn__16787 0x48ae2031 "alda.sound$play_BANG_$fn__16787@48ae2031"]

That leaves the problem of playing only the new notes, without the offset issue you observed...

The offset issue

It occurred to me that the alda.now namespace has some convenience functions/macros for building a score a little at a time, like we're doing here.

We can use with-score and provide the parsed history score as context, and then play just the new events, and it works out of the box without having to fiddle with the offsets:

; continuing the previous REPL session...
alda.lisp=> (now/with-score (atom (-> (score) (continue history)))
       #_=>   (now/play! new-input))
Jan 31, 2017 7:34:36 AM com.jsyn.engine.SynthesisEngine start
INFO: Pure Java JSyn from www.softsynth.com, rate = 44100, RT, V16.7.3 (build 457, 2014-12-25)
#object[clojure.lang.Atom 0x310cdd0f {:status :ready, :val {:chord-mode false, :current-instruments #{"piano-CXHnx"}, :events #{#alda.lisp.model.records.Note{:offset 500.0, :instrument "piano-CXHnx", :volume 1.0, :track-volume 0.7874015748031497, :panning 0.5, :midi-note 62, :pitch 293.6647679174076, :duration 450.0, :voice nil} #alda.lisp.model.records.Note{:offset 0, :instrument "piano-CXHnx", :volume 1.0, :track-volume 0.7874015748031497, :panning 0.5, :midi-note 60, :pitch 261.6255653005986, :duration 450.0, :voice nil} #alda.lisp.model.records.Note{:offset 1000.0, :instrument "piano-CXHnx", :volume 1.0, :track-volume 0.7874015748031497, :panning 0.5, :midi-note 64, :pitch 329.6275569128699, :duration 450.0, :voice nil}}, :beats-tally nil, :instruments {"piano-CXHnx" {:octave 4, :current-offset #alda.lisp.model.records.AbsoluteOffset{:offset 1500.0}, :key-signature {}, :config {:type :midi, :patch 1}, :duration {:beats 1, :ms nil}, :min-duration nil, :volume 1.0, :last-offset #alda.lisp.model.records.AbsoluteOffset{:offset 1000.0}, :id "piano-CXHnx", :quantization 0.9, :duration-inside-cram nil, :tempo 120, :panning 0.5, :current-marker :start, :time-scaling 1, :stock "midi-acoustic-grand-piano", :track-volume 0.7874015748031497}}, :markers {:start 0}, :cram-level 0, :global-attributes {}, :nicknames {}, :beats-tally-default nil}}]

One quirk of the alda.now API is that it expects the score passed to with-score to be an atom so that it can be updated in-place when each new snippet of code is played. We can work around that requirement by wrapping our history score in an atom: (atom (-> score (continue history)))

daveyarwood commented 7 years ago

As an aside, the work I'm doing right now on https://github.com/alda-lang/alda-core/issues/20 is going to touch on this idea that the parser should not be responsible for validating where events can and cannot go. I'm planning on adding some validation in the alda.lisp namespace, throwing exceptions at score evaluation-time for things that the parser would catch currently, like using part-specific events outside of a part.

For now, I think it's fine for the new Alda REPL to ignore things like that, and just parse everything (both history and new input) into events that we can work with using alda.now.

jgkamat commented 7 years ago

This seems perfect! Thanks!

I think that both the offset issue and the history being parsed incorrectly problem has been solved.

The parse-to-events-with-context function returns different things depending on the input, so I had to be super careful to handle all the cases. The cases that I think I got were:

empty input: returns (), don't dissoc events '\n': returns nil; don't dissoc events and convert to () letters, no instrument: returns non-map, don't dissoc

Let me know if there are any other edge cases that you could see. I'd rather test this for a bit (while I develop the repl) before this is done, just to be safe.

daveyarwood commented 7 years ago

This is looking good! I'll do some testing too when I have some time, and see if I can find any more edge cases.

daveyarwood commented 7 years ago

@jgkamat I spent some time playing with this today and found a couple bugs. I was able to get them fixed by using the with-score approach I described above. I think the server changes should be good to go! I tested by building the Java client (with your changes adding the -i/--history option) and running things like:

$ /tmp/alda -p 27714 play -i 'piano: c d e' -c 'f g a b > c'
$ /tmp/alda -p 27714 play -i 'trumpet: (tempo 300)' -c 'c d e'

I'm going to merge this change (via #6) and deploy new versions of alda-server-clj and alda-client-java to Clojars.