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

Remove inline context #1916

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

Remove inline context #1916

wants to merge 6 commits into from

Conversation

briannoyes
Copy link
Contributor

@briannoyes briannoyes commented Oct 25, 2024

Remove InlineContext from Agent definitions and orchestration usage

Fixes #{bug number} (in this specific format)
https://dev.azure.com/solliance/FoundationaLLM/_boards/board/t/FoundationaLLM%20Team/Stories/?workitem=19026
https://dev.azure.com/solliance/FoundationaLLM/_boards/board/t/FoundationaLLM%20Team/Stories/?workitem=19223

Details on the issue fix or feature implementation

Removed InlineContext (inline_context in Python and Vue code) from agent definitions

Confirm the following

  • I started this PR by branching from the head of the default branch
  • I have targeted the PR to merge into the default branch
  • This PR needs to be cherry-picked into at least one release branch
  • I have included unit tests for the issue/feature
  • I have included inline docs for my changes, where applicable
  • I have successfully run a local build
  • I have provided the required update scripts, where applicable
  • I have updated relevant docs, where applicable

Note

Instead of adding X's inside the checkboxes you wish to check above, first submit the PR, then check the boxes in the rendered description.

@@ -206,7 +206,7 @@ def _validate_request(self, request: KnowledgeManagementCompletionRequest):
if self.prompt.prefix is None or self.prompt.prefix == '':
raise LangChainException("The Prompt object provided in the request's objects dictionary is invalid because it is missing a prefix value.", 400)

if request.agent.vectorization is not None and not request.agent.inline_context:
if request.agent.vectorization is not None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change will break things. Most agent definitions have a vectorization object that is not null, but if it can often contain a text_embedding_profile_object_id that is null or ''. This should not result in an error, as these agents legitimately will not need vectorization.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kylebunting I'm unclear on what the check should do in combination with the nested checks and exception raises. Could you just make the appropriate corrections?

@@ -51,7 +51,7 @@ def _get_document_retriever(
"""
retriever = None

if agent.vectorization is not None and not agent.inline_context:
if agent.vectorization is not None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shoud also perform a check to verify the text_embedding_profile_object_id is not an empty string, which is the case for many agents.

@briannoyes
Copy link
Contributor Author

@kylebunting @ciprianjichici I made changes based on the conversation in standup today to make sure the removal of inline context checks did not result in errors where there should not be or null/empty indexing profile object ids.

Let me know if there are more changes you think are needed here

@briannoyes
Copy link
Contributor Author

@ciprianjichici This one is lingering. I have tested the Python changes and confirmed they are doing as expected, as well as the dotnet code of course.

@briannoyes
Copy link
Contributor Author

@ciprianjichici @codingbandit This one still lingering, do we want to merge this or is it OBE by now due to all the other pipeline changes?

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.

2 participants