Chat Inference Params

[noxware]

Changes

• Adds a toggle panel to configure the inference params of the chat.
• Adds customized slider and switch widgets.
• Make the existing chat panel Fill so it adapts better to the new size constraints.

Recording

Screen.Recording.2024-07-03.at.13.59.10.mp4

Notes for reviewers

• Figma design here.
• Most relevant file is chat_params.rs which is the panel itself.

[noxware]

Idk if there is a better way to have the text on the left and the switch on the right without a view.

[jmbejar]

I would say this is the right approach.

[jmbejar]

Is it possible that the slider text inputs are not working? I can sort of delete the numbers but I can not enter numbers manually at all.

[jmbejar]

I would say this is the right approach.

[jmbejar]

This is a good start, but we will probably want to have this settings separated by conversation (LMStudio and Jan does it, as a reference). But I'm OK with doing it in a separate iteration.

[noxware]

Oh sorry, I didn't understood well last time then.

I implemented per chat settings, the change is quick and small. ca1567b

Screen.Recording.2024-07-04.at.19.12.08.mov

But I had to track when the chat changes to ask makepad for a redraw (check the commit), and I don't like that. Do you have a different idea?

[jmbejar]

I presume this is necessary to avoid a situation where you are firing the toggle animation in this draw_walk, which cases to trigger redraws, so the animation would be restarted again (without having the change to make any progress).

Maybe it is better to implement this "data binding" relying more in retained mode? I just tested the following and seems to work:

    fn handle_event(&mut self, cx: &mut Cx, event: &Event, scope: &mut Scope) {
        if let Event::Startup = event {
            let store = scope.data.get::<Store>().unwrap();
            let ip = &store.chats.inferences_params;

            let temperature = self.slider(id!(temperature));
            let top_p = self.slider(id!(top_p));
            let max_tokens = self.slider(id!(max_tokens));
            let frequency_penalty = self.slider(id!(frequency_penalty));
            let presence_penalty = self.slider(id!(presence_penalty));
            let stop = self.text_input(id!(stop));
            let stream = self.check_box(id!(stream));

            temperature.set_value(ip.temperature.into());
            top_p.set_value(ip.top_p.into());
            max_tokens.set_value(ip.max_tokens.into());
            frequency_penalty.set_value(ip.frequency_penalty.into());
            presence_penalty.set_value(ip.presence_penalty.into());
            stop.set_text(&ip.stop);

            stream.set_selected(cx, ip.stream);
        }

        self.view.handle_event(cx, event, scope);
        self.widget_match_event(cx, event, scope);

        ...
    }
    
    fn draw_walk(&mut self, cx: &mut Cx2d, scope: &mut Scope, walk: Walk) -> DrawStep {
        self.view.draw_walk(cx, scope, walk)
    }

It is not a situation where the settings values could be changed from other sources, so it feels safe to make the local state source of truth after taking the data from the store at startup time.

Even thinking in the case of having this setting per-conversation, the form data can be easily repopulated from the store just reacting to the existing ChatHistoryCardAction::ChatSelected action.

[noxware]

You are firing the toggle animation in this draw_walk, which cases to trigger redraws, so the animation would be restarted again

Yeah, I had that theory for the checkbox widget when I experienced this other thing in the slider one. Although, I still don't understand why it worked on empty chats.

I probably had to update that comment at least saying "this is because setting the value also activates the animation as a side effect causing visual issues so we avoid calling it".

Maybe it is better to implement this "data binding" relying more in retained mode? I just tested the following and seems to work:

I tried the same when I hit the switch problem the first time. But I discarded the commit because of the following:

• We are changing the approach just because one and only one widget is miss behaving.
  • Fixed with a simple if (2 harmless lines).
• We lose proper data binding in exchange of incomplete data binding.
  • Although, as you mention, it works in this current case, now everyone must know to NOT mutate this from the store directly or they would introduce a bug with not immediate visual implications (risky). Yeah we can put a comment saying "hey, this comes from X widget, do not mutate this directly" but we can also not have that risky point of failure on the first place.
• That solution, or the if solution. Both are workarounds. Both are just required because of the switch. Both are not required otherwise. And one involves less functional changes and is more robust than the other.
• If we want retained mode, we don't need data binding on the first place, we should directly ask the widget it's data when we need it. Something like one of the following approaches:

• And I tried doing the previous point, to have no-dirty, no-fragile, robust retained mode UI, but I stopped in the middle because I was like... "hey... I'm seriously changing my whole implementation just because of this dumb switch...?"
  • ...Thanks god I found how to fix the switch.

So, this would be my course of action:

• Let's leave the thing how it's currently done. I don't see any problem with the implementation. Don't touch it. It works. said a sage... somewhere... probably...
  • But let's update this comment giving the animation explanation.
• Unless
  • We prefer going fully retained as an agreement.
  • We found an undetected, not easy to resolve issue with this and we MUST go retained, in which case:
    • I would prefer the approach from the diagrams.
    • But... I'm lazy... So I would actually pick your suggestion and add a warning comment in the store and call it a day.

[noxware]

Is it possible that the slider text inputs are not working?

@jmbejar I tried at c0980c86d of the makepad repo (before rik merged by branch) with a simple example but it doesn't work either.

Screen.Recording.2024-07-04.at.17.34.47.mp4

So rest assured, we didn't introduce any issue :P