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

added Marimo support for echo=True #981

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

peterwilli
Copy link

@peterwilli peterwilli commented Aug 14, 2024

Marimo is an alternative to Jupyter Lab, it contains a reactive UI and self-executing cells. I encourage people to check it out: https://marimo.io

However, the output of Guidance in Marimo is very scrambled with HTML, I altered the debug streamer to take Marimo into account, and use Marimo's own format to display the HTML output.

You can see the difference in the old version (left) and after this PR (right)

Comparison of before and after the PR

I also made sure that if someone has Jupyter installed + Marimo, that Jupyter printing will be preferred, this is to make sure that the original behaviour will not change and that current Jupyter users will not have any surprises.

How to test

I created a test Marimo script:

import marimo

__generated_with = "0.7.19"
app = marimo.App(width="medium")


@app.cell
def __():
    import marimo as mo
    from guidance import models, gen
    from guidance.chat import ChatTemplate
    from guidance import user, system, assistant
    from transformers import AutoTokenizer
    return (
        AutoTokenizer,
        ChatTemplate,
        assistant,
        gen,
        mo,
        models,
        system,
        user,
    )


@app.cell
def __(AutoTokenizer):
    model_id = "meta-llama/Meta-Llama-3.1-8B-Instruct"
    tokenizer = AutoTokenizer.from_pretrained(model_id)
    return model_id, tokenizer


@app.cell
def __(ChatTemplate, UnsupportedRoleException, models, tokenizer):
    # Workaround (see: https://github.com/guidance-ai/guidance/discussions/917)
    class Llama31ChatTemplate(ChatTemplate):
        template_str = tokenizer.chat_template

        def get_role_start(self, role_name):
            if role_name == "system":
                return "<|begin_of_text|><|start_header_id|>system<|end_header_id|>\n\n"
            elif role_name == "user":
                return "<|start_header_id|>user<|end_header_id|>\n\n"
            elif role_name == "assistant":
                return "<|start_header_id|>assistant<|end_header_id|>\n\n"
            else:
                raise UnsupportedRoleException(role_name, self)

        def get_role_end(self, role_name=None):
            return "<|eot_id|>"


    model = models.LlamaCpp(
        "/Users/peter/Downloads/models/llama3.1-instruct.gguf",
        n_ctx=8192,
        chat_template=Llama31ChatTemplate,
        echo=True,
    )
    return Llama31ChatTemplate, model


@app.cell
def __(assistant, gen, model, system, user):
    lm = model
    with system():
        lm += "You are someone that loves dragons and arts, you also are a big fan of the GitHub user hudson-ai."
    with user():
        lm += "What is love?"
    with assistant():
        lm += gen()
    return lm,


if __name__ == "__main__":
    app.run()

(I have no affiliation with Marimo, just wanted to use it with Guidance!)

@hudson-ai
Copy link
Collaborator

Thanks @peterwilli ! This looks like a nice and straight-forward addition.

@riedgar-ms I don't think it's necessary for this PR, but this provides further evidence that we need a "display handler"
abstraction rather than doing all of the output logic here. Maybe you and I can brainstorm how/whether that fits with #929

@peterwilli
Copy link
Author

I agree that an abstraction layer (display handler) is the best way forward, and thanks by the way!

I could extend this pr into a display handler if needed

@peterwilli peterwilli force-pushed the feat/marimo_logging branch 2 times, most recently from 9d5d8e9 to d7edfaf Compare August 21, 2024 16:04
@peterwilli peterwilli force-pushed the feat/marimo_logging branch from d7edfaf to 76e7b36 Compare August 21, 2024 16:05
@hudson-ai
Copy link
Collaborator

hudson-ai commented Sep 3, 2024

@peterwilli sorry for taking so long to get back to you! I was on vacation and not keeping super up to date. No need to extend this to a display handler for the sake of this PR imo. (But internally, let's definitely have more discussions around this @riedgar-ms)

@Harsha-Nori just want to clear this with you before approving/merging -- I think we tend to think of Jupyter as a pretty "first-class citizen". Not sure how wide we want to get with support for alternatives, but I feel that this change is so light weight that we're still pretty far from any such slippery slope. Thoughts?

@codecov-commenter
Copy link

codecov-commenter commented Sep 3, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 58.33333% with 5 lines in your changes missing coverage. Please review.

Project coverage is 61.12%. Comparing base (8d63c79) to head (76e7b36).
Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
guidance/models/_model.py 58.33% 5 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #981       +/-   ##
===========================================
+ Coverage   44.74%   61.12%   +16.38%     
===========================================
  Files          62       62               
  Lines        4392     4404       +12     
===========================================
+ Hits         1965     2692      +727     
+ Misses       2427     1712      -715     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

3 participants