feat: add guards to session management #101

lamchau · 2024-09-27T09:52:39Z

added a fix/workaround for rich color handling
switched [dim] to [bold] for dark terminals
added a cleanup for empty sessions: edge case when a user aborts early - without it empty sessions are generated so we're unable to find the "right" last valid session to resume
added a prompt menu for overwrites: added a check for existing session files and give a user an opportunity to recover before they take a destructive action

note: the image shows both the empty session file (goose session list doesn't contain abcd.jsonl) and what the menu ux looks like for a session conflict

rich's parser gets confused when there's a color string so we need to add a space otherwise the coloring ends up getting mashed.

lamchau · 2024-09-27T09:54:57Z

src/goose/cli/session.py

@@ -140,13 +140,45 @@ def process_first_message(self) -> Optional[Message]:
            return Message.user(text=user_input.text)
        return self.exchange.messages.pop()

+    def prompt_overwrite_session(self) -> None:
+        print(f"[yellow]session already exists at {self.session_file_path}.[/]\n")
+        print("would like to overwrite the existing session?")


could use a second opinion on this menu, is there a way we can improve the ux?

renamed this function to _prompt_overwrite_session in a1c8c82 (still could use the feedback)

lamchau · 2024-09-27T10:09:13Z

src/goose/cli/session.py

+            self.prompt_overwrite_session()
+
+        print(
+            f"[bold]starting session | name: [cyan]{self.name}[/]  profile: [cyan]{self.profile or 'default'}[/][/bold]"


@lifeizhou-ap i added some screenshots in the body of the PR to to make this bold since it was hard to see with dark themes on my terminal. happy to change this back!

keep it dim imo! this info shows every time in case you need it but you mostly don't, so i like it being de-emphasized

lifeizhou-ap · 2024-10-01T05:34:17Z

Hi @lamchau,

Nice one!

I noticed that the session.py file is getting bigger. Just wondering whether it is better to separate some of these logics to another component or file?

lifeizhou-ap · 2024-10-01T05:38:22Z

also I feel that with your change, we may not require session resume as user can resume the session via session start. WDYT?

lamchau · 2024-10-01T05:40:32Z

@lifeizhou-ap maybe after we get to 500 lines or so unless you have another suggestion?

30% or so of the Session class is text strings (split lines) and docstrings - with code folding it feels digestible. i'm worried if we start to make it to abstract it requires more context switching/modification across files

lamchau · 2024-10-01T05:55:19Z

[formatted for clarity]

@lifeizhou-ap possibly! but the challenge lies in how it's invoked in this project and internally as a subcommand and how it's used via pipx

xx goose session start <tab> doesn't autocomplete session names
- xx doesn't have shell completions so click can't autocomplete on <tab> we would need to add shell completions to xx (in golang)
- a user would need 4 steps
1. xx goose session list (get top)
2. xx goose session start <session_name>
3. prompt with conflict
4. r to resume

with pipx it would still require a conflict for a resume to happen (e.g. goose session start) creates a generated session each time, so that requires the user to use <tab> to autocomplete the latest one

goose session start <tab>
choose session with conflict
r to resume

if we keep the command xx goose session resume and goose session resume it is only 1 step - this convenience is essentially "free" and saves a few steps for the user.

anyhoo, those are my thoughts/biases - we should go with whatever flow makes the most sense. since it's a product decision, maybe anna can make the call?

lifeizhou-ap · 2024-10-01T23:04:46Z

@lifeizhou-ap maybe after we get to 500 lines or so unless you have another suggestion?

30% or so of the Session class is text strings (split lines) and docstrings - with code folding it feels digestible. i'm worried if we start to make it to abstract it requires more context switching/modification across files

I see. Personally I prefer small files for readability and maintenance. However, I understand your concern and we can refactor in the future when it reaches the point for refactoring.

One suggestion: Maybe extract the code into a function?

# prevents cluttering the `sessions` with empty files, which
        # can be confusing when resuming a session
        if is_empty_session(self.session_file_path):
            try:
                self.session_file_path.unlink()
            except FileNotFoundError:
                pass
            except Exception as e:
                raise Exception(f"error deleting empty session file: {e}")

lifeizhou-ap · 2024-10-02T00:04:29Z

Hey @baxen I am happy with this PR (only with a small suggestion). Would you like to review it?

lamchau · 2024-10-02T04:13:46Z

@lifeizhou-ap sure thing, extracted and reordered them here a1c8c82

baxen

LGTM

baxen · 2024-10-04T02:37:12Z

src/goose/cli/session.py

+            self.prompt_overwrite_session()
+
+        print(
+            f"[bold]starting session | name: [cyan]{self.name}[/]  profile: [cyan]{self.profile or 'default'}[/][/bold]"


keep it dim imo! this info shows every time in case you need it but you mostly don't, so i like it being de-emphasized

src/goose/cli/session.py

lamchau · 2024-10-04T19:06:05Z

keep it dim imo! this info shows every time in case you need it but you mostly don't, so i like it being de-emphasized

@baxen it's pretty hard to see dim magenta, it'd be friendlier imo if we either keep it the same gray color and not emphasize it with color at all. or alternatively we don't display the file location at all?

baxen · 2024-10-08T17:39:15Z

keep it dim imo! this info shows every time in case you need it but you mostly don't, so i like it being de-emphasized

@baxen it's pretty hard to see dim magenta, it'd be friendlier imo if we either keep it the same gray color and not emphasize it with color at all. or alternatively we don't display the file location at all?

+1 to not needing tho show the file location at all here

lamchau · 2024-10-10T11:33:53Z

@baxen for posterity

reverted dim a1d4bba
removed session location c3da1a4

* main: feat: add groq provider (#134) feat: add a deep thinking reasoner model (o1-preview/mini) (#68) fix: use concrete SessionNotifier (#135) feat: add guards to session management (#101) fix: Set default model configuration for the Google provider. (#131) test: convert Google Gemini tests to VCR (#118) chore: Add goose providers list command (#116) docs: working ollama for desktop (#125) docs: format and clean up warnings/errors (#120) docs: update deploy workflow (#124) feat: Implement a goose run command (#121)

lamchau added 3 commits September 27, 2024 02:39

fix: correct colorization

926e60c

rich's parser gets confused when there's a color string so we need to add a space otherwise the coloring ends up getting mashed.

feat: use bold instead of dim

76507ef

fix: prevent creating empty sessions

958726d

lamchau requested review from lifeizhou-ap and baxen September 27, 2024 09:52

lamchau commented Sep 27, 2024

View reviewed changes

lamchau added 3 commits September 27, 2024 03:28

fix: prevent overwriting existing sessions

04a3a8e

test: add tests to validate content

26b3a6d

refactor: move static functions to utils

a43e65e

lamchau force-pushed the lam/fix-color branch from 88efdc0 to a43e65e Compare September 27, 2024 10:28

refactor: reduce nesting context

9d0c8c8

lifeizhou-ap approved these changes Oct 2, 2024

View reviewed changes

refactor: reorder functions

a1c8c82

baxen approved these changes Oct 4, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into lam/fix-color

5efaba2

lamchau added 5 commits October 10, 2024 03:56

refactor: use rich.prompt

aa289ca

style: update with primitive type hints

96e9cd7

style: sort imports

986a8e9

fix: remove redundant session location

c3da1a4

style: revert color dim, fix closing tags

a1d4bba

Merge branch 'main' into lam/fix-color

e6a428c

tests: refactor using patch decorator

93859ed

lamchau force-pushed the lam/fix-color branch from 20dec22 to 93859ed Compare October 10, 2024 11:49

tests: add descriptive name for patch order

f8fce2f

lamchau force-pushed the lam/fix-color branch from 82058e1 to f8fce2f Compare October 10, 2024 11:57

lamchau merged commit 4375e2f into block:main Oct 10, 2024
4 checks passed

lamchau deleted the lam/fix-color branch October 10, 2024 12:01

lamchau mentioned this pull request Oct 22, 2024

fix: correct context loading from session new/overwrite and resume #180

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add guards to session management #101

feat: add guards to session management #101

lamchau commented Sep 27, 2024 •

edited

Loading

lamchau Sep 27, 2024

lamchau Oct 2, 2024 •

edited

Loading

lamchau Sep 27, 2024

baxen Oct 4, 2024

lifeizhou-ap commented Oct 1, 2024

lifeizhou-ap commented Oct 1, 2024

lamchau commented Oct 1, 2024

lamchau commented Oct 1, 2024 •

edited

Loading

lifeizhou-ap commented Oct 1, 2024

lifeizhou-ap commented Oct 2, 2024

lamchau commented Oct 2, 2024

baxen left a comment

baxen Oct 4, 2024

lamchau commented Oct 4, 2024

baxen commented Oct 8, 2024

lamchau commented Oct 10, 2024

feat: add guards to session management #101

feat: add guards to session management #101

Conversation

lamchau commented Sep 27, 2024 • edited Loading

lamchau Sep 27, 2024

Choose a reason for hiding this comment

lamchau Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

lamchau Sep 27, 2024

Choose a reason for hiding this comment

baxen Oct 4, 2024

Choose a reason for hiding this comment

lifeizhou-ap commented Oct 1, 2024

lifeizhou-ap commented Oct 1, 2024

lamchau commented Oct 1, 2024

lamchau commented Oct 1, 2024 • edited Loading

lifeizhou-ap commented Oct 1, 2024

lifeizhou-ap commented Oct 2, 2024

lamchau commented Oct 2, 2024

baxen left a comment

Choose a reason for hiding this comment

baxen Oct 4, 2024

Choose a reason for hiding this comment

lamchau commented Oct 4, 2024

baxen commented Oct 8, 2024

lamchau commented Oct 10, 2024

lamchau commented Sep 27, 2024 •

edited

Loading

lamchau Oct 2, 2024 •

edited

Loading

lamchau commented Oct 1, 2024 •

edited

Loading