-
Notifications
You must be signed in to change notification settings - Fork 49
refactor: Full refactor of the Decompose CLI Tool & introduction of prompt_modules #105
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
Conversation
|
@HendrikStrobelt This is Tulio's new prompt decomposition pipeline. You do not need to review the full PR -- it's a lot of code, and Tulio gave me a high-level overview. But do please chime in on where this code should live. A few options:
|
|
The failed test seems like a false negative. @tuliocoppola can you run the python 3.12 tests locally and confirm they pass there? |
@nrfulton |
|
@tuliocoppola Overall LGTM. Can you move the code to Once that's done we can squash and merge. |
|
@nrfulton |
|
@tuliocoppola, this looks good; can you please move |
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟢 Enforce conventional commitWonderful, this rule succeeded.Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
And sorry for the back-and-forth here... my fault, and this is really the last move request. |
Signed-off-by: Tulio Coppola <tulio.cppl@icloud.com>
Signed-off-by: Tulio Coppola <tulio.cppl@icloud.com>
|
@jakelorocco & @nrfulton |
jakelorocco
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm (except for the one thing noted below @tuliocoppola); we chatted and agree that longterm, we will work on porting this code to be more melleaic; for now, keeping it in the cli directory should be good
I was able to run the command, so it looks good
| model_id=model_id, | ||
| model_options={ | ||
| ModelOption.CONTEXT_WINDOW: 32768, | ||
| "timeout": backend_req_timeout, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine to leave this in for now, but I believe ollama actually sets this as an environment variable. Error from my ollama logs:
time=2025-09-04T15:16:14.510-04:00 level=WARN source=types.go:654 msg="invalid option provided" option=timeout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakelorocco
There's an argument when spinning up the client locally with ollama where you can define the request timeout for that client:
https://github.com/ollama/ollama-python/blob/main/ollama/_client.py#L81
So this option would need to be mapped on these lines I guess:
https://github.com/generative-computing/mellea/blob/main/mellea/backends/ollama.py#L67-L68
We can open an issue for this to be implemented later.
I guess that, for now, I can comment that timeout and add a TODO comment to re-enable it when it's supported by the backend class. Since this will probably have to be made available through the special ModelOption thing.
| case DecompBackend.rits: | ||
| assert backend_endpoint is not None, ( | ||
| 'Required to provide "backend_endpoint" for this configuration' | ||
| ) | ||
| assert backend_api_key is not None, ( | ||
| 'Required to provide "backend_api_key" for this configuration' | ||
| ) | ||
|
|
||
| from mellea_ibm.rits import RITSBackend, RITSModelIdentifier # type: ignore | ||
|
|
||
| m_session = MelleaSession( | ||
| RITSBackend( | ||
| RITSModelIdentifier(endpoint=backend_endpoint, model_name=model_id), | ||
| api_key=backend_api_key, | ||
| model_options={"timeout": backend_req_timeout}, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also remove this and somehow add it through our internal library. We don't want to expose this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tuliocoppola, sorry should've spotted this earlier; last change request!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakelorocco
I've actually went through this specific matter with @nrfulton, we basically agreed that there's no harm, the service name is not a trade secret or anything, and there's no implementation code exposed, it's just 2 class names.
I'm not sure how we could make this work easily if not like that, and we do need to support this backend asap for the decomposition pipeline.
One thing that might be good is to omit the "rits" option from the CLI's "--backend", and trigger it by providing an in-line environment variable like this:
BACKEND_OVERRIDE=rits m decompose run ...
Just for people to not mistakenly try to use it when looking at the possible values on the CLI's help strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you know an easy way for making this work through the internal library, then we can do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh okay; if it's already been addressed, I'm fine with it as is. I agree that it's not exposing implementation details here.
…rompt_modules (generative-computing#105) * Implements "prompt_modules" and complete refactor of the "decompose" feature * typo: missing period * minor fix: changed the "NotRequired" import * fix: minor fixes * moves prompt_modules to utils * moves decompose modules to appropriate path * refactor: moves prompt_modules to cli scope Signed-off-by: Tulio Coppola <tulio.cppl@icloud.com> * adds README.md to write later Signed-off-by: Tulio Coppola <tulio.cppl@icloud.com> --------- Signed-off-by: Tulio Coppola <tulio.cppl@icloud.com> Co-authored-by: Tulio Coppola <tuliocoppola@ibm.com> Co-authored-by: Nathan Fulton <nathan@ibm.com>
This PR was discussed/showed to @nrfulton.
It introduces the concept of
prompt_modulesunder thehelpers, which are completely isolated modules, with a common interface, that can be composed into any kind of pipeline.These modules are the blocks supporting the Decomposition Pipeline implemented under the CLI tools.
It's a lot of code, but all the introduced files are isolated under the new directory @
mellea/helpers/prompt_modules, and none of them interfere with any other part of the code base.There's also modifications to the
cli/decompose.Usage example for one of the
prompt_modules:Now an example of the Decomposition CLI Tool usage:
For this prompt text file:
my-task-prompt.txtThe Decomposition CLI Tool supports task prompts that doesn't need any user input data, but also support the ones that need.
Note that the example task prompt above needs 2 user input variables to execute its task, it does not explicitly dictates the input variables names.
On the CLI command you must pass a descriptive name for each necessary user input variable, in this example we will call our variables
NEWS_ARTICLEandCELEBRITY_NAME, but we could've just as well called themINPUT_ARTICLEandCELEBRITY_DATA, as you can see, the idea is to pass descriptive names for the LLM to correctly infer and reference the input data when generating the subtasks templates.If your task prompt doesn't need user input data, then you can just omit the
--input-varoptions.Usage example of the new Decomposition Pipeline CLI Tool: