-
Notifications
You must be signed in to change notification settings - Fork 241
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
Update module4, research assistant #34
base: main
Are you sure you want to change the base?
Conversation
Also @shiv248 this is an interesting one that came up on Module 4. For some models, messages need to alternate AI -> Human. This should resolve. Also, this adds an input filter to Studio schema in Module 4, which is nice for visualization. |
my review notes, obviously I'll use my local model again :) (updating these live over the next bit of time): 1 just a prompt engineering issue
the change to this prompt (the content property) actually makes it harder for my smallish models (mistal-nemo, but also qwen-2.5 instruct 32b) to succeed with the tool call to fill this structured output. So we get:
2 minor code change and prompt engineering issuedef search_wikipedia(state: InterviewState):
""" Retrieve docs from wikipedia """
# Search query
...
from IPython.display import Markdown
thread = {"configurable": {"thread_id": "1"}}
interview = interview_graph.invoke({"analyst": analysts[0], "messages": [], "max_num_turns": 2}, thread)
Markdown(interview['sections'][0]) is giving me:
this appears to be related to the messages being used during invocation. This works: from IPython.display import Markdown
messages = [HumanMessage(f"So you said you were writing an article on {topic}?")]
thread = {"configurable": {"thread_id": "1"}}
interview = interview_graph.invoke({"analyst": analysts[0], "messages": messages, "max_num_turns": 3}, thread)
Markdown(interview['sections'][0]) But then the tavily search happens to fail. I see a human message is coming back blank so, I'm not sure at the moment why. edit: I forgot I had rewritten these prompts in the preceding graph, its not just search instructions. Although it is not necessary, prompt engineering is a part of the journey especially for local models, please feel free to lift my prompts from the notebook I sent you. |
Hmm.. this is interesting. It seems that the adjustment of inclusive to general models is causing issues within the ipynb for some? @robbiemu. I just tried @rlancemartin research assistant notebook in this branch might have to dig deeper into this. |
hi Shiv, I was asked to give this a spin to see if it was more compatible with local models (I had gotten the original working with local models and written somewhere here about it). It essentially is, there's one code change outside of prompts that I see mattering. |
i didn't test w/ Gemini, actually. but i do think these changes will enable compatibility w/ any models (Anthropic I know has this requirement, but also some Fireworks-hosted model and Gemini) that require alternating user-assistant messages. |
ya, local models will remain a problem for other reasons beyond alternating message types. |
for local models -- @robbiemu what would help: do you mind putting up a PR with the precise prompt changes (or other changes) needed? i can test to -- 1/ make sure it STILL works w/ OAI (default for the course) if there are code changes that break 1, then i can't merge but i can simply link to the closed PR in a issue we'll create to explain how to work with local LLMs. |
imo, the issue with opening up more accessibility for specific local solutions on this repo through using ChatOllama or ChatLlamacpp is it is highly model-dependent and the variance of parameters is always going to be there. I want to think the goal of this repo as it grows is to be as smooth as possible for learners as they work through concepts and try those concepts/paradigms out with different approaches, without having to deal with unexpected issues. Kind of like a i do like the idea of having a simple ipynb/wiki demo of how to convert from commercial models to local models, and a brief overview of what hurdles they might face. |
I don't disagree with this. My initial comment was that the structure (not using AIMessages as final messages) often lead local models to not respond and that this was an issue even with the mistral models I run locally (this would remain so when using the mistral API). Fixing that (much as I did in my local notebooks) I believe was the motivation for this PR. The rest is just prompt engineering, which we can expect to change over time. The one code change I suggested above was to mirror details from the video that happen to greatly improve interviews on my local models .. but ultimately this could probably also be resolved with prompt engineering. However, since the solution that I have on hand is not strictly prompt engineering, I felt it worth noting in my review above. if you all decide you are in agreement and interested in my creating a PR for the prompts (and that code change I indicated above) I am happy to provide it. Let me know. |
ah my apologies @robbiemu, that's understandable, if it is just prompt engineering changes, and the OAI models still work, I think it is a step in the right direction! I would also in the PR mention how you, yourself, switch between (ie changes you make to) the OAI integration within the notebooks to your local model solutions and your thoughts about which ones you like (model & B parameter) and have found most successful with as you work through the course. That way we can always link that PR # to future issues that might occur or users interested in the same thing. |
1/ Add alternating Human -> AI messages in interview (
generate_question
should be aHumanMessage
) for compatibility w/ some LLMs that enforce it.2/ Update Tavily.
3/ Also add an input schema for the research assistant graph in Studio.