feat: simplify intrinsics (code and examples)#946
Conversation
Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com> Assisted-by: CLAUDE:OPUS
Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com> Assisted-by: CLAUDE:OPUS
Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com> Assisted-by: CLAUDE:OPUS
Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com> Assisted-by: CLAUDE:OPUS
Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com>
Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com>
|
The PR description has been updated. Please fill out the template for your PR to be reviewed. |
Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com> Assisted-by: CLAUDE:OPUS
ajbozarth
left a comment
There was a problem hiding this comment.
Three inline comments below. General notes:
Circular import pattern (start_backend.py ↔ session.py): session.py does a top-level import of start_backend, while start_backend.py defers its import of _resolve_backend_and_context inside the function body to avoid the cycle. Works at runtime, but fragile — the impl file depends on a private helper from another module. Cleaner to move _resolve_backend_and_context / _resolve_context into a _session_util.py that both files import independently.
Merge surface with #881: Both PRs touch _util.py, rag.py, and all the docs/examples/intrinsics/ files. The changes are orthogonal (this PR adds the coerce/resolve helpers; #881 extends call_intrinsic to support OpenAIBackend) but the merge will need care to keep both. Also: citations.py here drops the explicit doc_id="1" / doc_id="2" in favour of auto_doc_id=True (0-indexed). #881 also modifies that file — verify the final merged example has consistent IDs and matches whatever the intrinsic test fixtures expect.
Return type fixes — rewrite_question and check_context_relevance were both typed -> float; now correctly -> str. Good catches.
| ) | ||
| else: | ||
| model_id_str = str(model_id) | ||
| resolved_ctx, backend, model_id_str = _resolve_backend_and_context( |
There was a problem hiding this comment.
Regression: this constructs the backend (including passing model_options to the constructor) before the SESSION_PRE_INIT hook runs. In the original code the hook ran first and the backend was built with the hook's (potentially modified) model_options. Now the hook updates the local model_options variable on line 291 but the backend never sees it.
Fix: split this helper — call _resolve_context here for early argument validation, then construct the backend after the hook block.
There was a problem hiding this comment.
Resolved; split functionality up and calling the backend creation post hook.
…ssage for intrinsics Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com> Assisted-by: CLAUDE:OPUS
Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com> Assisted-by: CLAUDE:OPUS
Fixed the circular imports. Split up functionality and moved everything to start backend. We don't need a separate utils function. session can just import from start_backend.
I tested all intrinsic examples. Functionality is the same. I will resolve merge conflicts in the other PR. This PR takes precedence. |
ajbozarth
left a comment
There was a problem hiding this comment.
All three issues from the previous round are addressed — hook ordering fixed, unreachable assert removed, misleading comment removed. One remaining concern with no specific line to attach: the hook ordering fix has no regression test. A unit test that patches invoke_hook to mutate model_options and asserts the backend was constructed with the updated value would lock this in.
Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com>
|
@avinash2692 @ajbozarth, can I please get one (or more of y'all's) review on this? We are going to try to get some of these larger intrinsic changes merged today and then follow up with additional PRs if any issues are spotted before the release / with new models. |
ajbozarth
left a comment
There was a problem hiding this comment.
One minor non-blocking comment you missed in my previous review, but otherwise this LGTM.
the hook ordering fix has no regression test. A unit test that patches
invoke_hookto mutatemodel_optionsand asserts the backend was constructed with the updated value would lock this in.
Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com>
| Document( | ||
| doc_id="1", | ||
| text="Keith Rupert Murdoch was born on 11 March 1931 in Melbourne, Australia, " | ||
| ( |
There was a problem hiding this comment.
This change will cause a warning to be printed, after which the intrinsic's output will reference document IDs that the user did not provide. Please undo this change.
There was a problem hiding this comment.
Will undo this and utilize explicit documents / doc ids where necessary.
| ctx, backend = start_backend( | ||
| "hf", model_id=model_ids.IBM_GRANITE_4_MICRO_3B, context_type="chat" | ||
| ) | ||
| # NOTE: This example can also be run with the OpenAIBackend using a GraniteSwitch model. See docs/examples/granite-switch/. |
There was a problem hiding this comment.
docs/examples/granite-switch does not exist in this branch. Have the same updates been made to the contents of that directory on whatever branch it exists on?
There was a problem hiding this comment.
This might be some caching issue. It was added in another PR and then I updated this one to handle the merge conflicts with that message. The directory exists: https://github.com/generative-computing/mellea/tree/main/docs/examples/granite-switch
| print(f"Original user question: {next_user_turn}") | ||
|
|
||
| result = rag.rewrite_question(next_user_turn, context, backend) | ||
| result = rag.rewrite_question(None, ctx_with_question, backend) |
There was a problem hiding this comment.
Calling this intrinsic with the first argument folded into the context is inconsistent with the way the other examples work. Is there a reason to handle query rewrite differently?
There was a problem hiding this comment.
All of the intrinsics now support this. The other examples don't showcase this functionality since they rely on pre-canned input/output where applicable. The Mellea team wanted to have a succinct way to reference information that was already in the context and this example was the one most suited for it.
…o_doc_id for coerce_docs with intrinsics Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com>
Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com>
* feat: enhance start_session Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com> Assisted-by: CLAUDE:OPUS * feat: add typing to start_backend Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com> Assisted-by: CLAUDE:OPUS * feat: simplify intrinsic document parameters Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com> Assisted-by: CLAUDE:OPUS * feat: simplify intrinsic question/response parameters Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com> Assisted-by: CLAUDE:OPUS * feat: simplify mellea imports and intrinsic examples Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com> * fix: pre-commit Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com> * feat: get intrinsic auto-resolve from context working for non-Messages Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com> Assisted-by: CLAUDE:OPUS * feat: additional formatting capability for auto-grabbing last user message for intrinsics Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com> Assisted-by: CLAUDE:OPUS * fix: pr comments Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com> Assisted-by: CLAUDE:OPUS * fix: more pr comments Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com> * fix: pr comments: revert document handling when id matter; remove auto_doc_id for coerce_docs with intrinsics Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com> * fix: modify query rewrite example messages to make sense Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com> --------- Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com>
Misc PR
Type of PR
Description
I will likely have to fix merge conflicts between this and the other granite switch pr. I would ideally like to merge this first.
Changes:
Testing
Attribution