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.

Sign up for GitHub

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

Jump to bottom

feat(agents-api): Add label lookup for inputs and outputs #906

Draft
wants to merge 2 commits into
base: dev
Choose a base branch
from
Draft

feat(agents-api): Add label lookup for inputs and outputs #906

wants to merge 2 commits into from
+21 −7

Conversation

Ahmad-mtos
Copy link
Contributor

@Ahmad-mtos Ahmad-mtos commented Nov 28, 2024 •
edited by qodo-merge-pro-for-open-source bot
Loading

User description

closes #791

Important

Add label lookup for inputs and outputs in agents API by updating StepContext and workflow execution methods.

  • Behavior:
    • Add labels attribute to StepContext in tasks.py for label lookup in inputs and outputs.
    • Update prepare_for_step in tasks.py to merge labels with inputs and outputs.
  • Workflow Execution:
    • Modify run method in __init__.py to handle previous_labels for task execution.
    • Update continue_as_child in helpers.py to accept previous_labels parameter.

This description was created by Ellipsis for 9a3cfd0. It will automatically update as commits are pushed.

PR Type

Enhancement

Description

  • Added labels attribute to StepContext for label-based input/output lookup.

  • Updated prepare_for_step to merge labels with inputs and outputs.

  • Modified run method to handle previous_labels for task execution.

  • Enhanced continue_as_child to accept and pass previous_labels.

Changes walkthrough 📝

Relevant files
Formatting
prompt_step.py
Cleaned up redundant imports in `prompt_step.py`                 

agents-api/agents_api/activities/task_steps/prompt_step.py

  • Removed redundant imports for better code clarity.
  • Simplified import structure by eliminating duplicates.
    		• +0/-2     
    chat.py
    Simplified and cleaned up imports in `chat.py`                     

    agents-api/agents_api/routers/sessions/chat.py

  • Removed duplicate imports for better readability.
  • Streamlined import statements for improved maintainability.
    		• +0/-5     
    Enhancement
    tasks.py
    Introduced label-based input/output handling in `StepContext`

    agents-api/agents_api/common/protocol/tasks.py

  • Added labels attribute to StepContext for label lookup.
  • Updated prepare_for_step to merge labels with inputs and outputs.
  • Enhanced input/output handling with label-based mapping.
    		• +10/-0   
    __init__.py
    Integrated label handling into task execution workflow     

    agents-api/agents_api/workflows/task_execution/init.py

  • Added previous_labels parameter to run method.
  • Updated workflow logic to handle and propagate labels.
  • Enhanced label-based output mapping in task execution.
    		• +9/-0     
    helpers.py
    Added label propagation support in `continue_as_child`     

    agents-api/agents_api/workflows/task_execution/helpers.py

  • Added previous_labels parameter to continue_as_child.
  • Updated helper function to support label propagation.
    		• +2/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @Ahmad-mtos Ahmad-mtos requested a review from creatorrr November 28, 2024 15:44
    ellipsis-dev[bot]
    ellipsis-dev bot reviewed Nov 28, 2024
    View reviewed changes
    Copy link
    Contributor

    @ellipsis-dev ellipsis-dev bot left a comment

    Choose a reason for hiding this comment

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

    ❌ Changes requested. Reviewed everything up to 9a3cfd0 in 41 seconds

    More details
    • Looked at 102 lines of code in 3 files
    • Skipped 0 files when reviewing.
    • Skipped posting 1 drafted comments based on config settings.
    1. agents-api/agents_api/workflows/task_execution/__init__.py:140
    • Draft comment:
      Initializing previous_labels as an empty dictionary if not provided is consistent with the changes for label lookup. This logic is correct.
    • Reason this comment was not posted:
      Confidence changes required: 0%
      The run method in TaskExecutionWorkflow class initializes previous_labels as an empty dictionary if not provided. This is consistent with the changes made to support label lookup for inputs and outputs. The logic seems correct here.

    Workflow ID: wflow_iVQFZayAMbHKoGjo

    Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

    agents-api/agents_api/common/protocol/tasks.py Show resolved Hide resolved
    @creatorrr
    Copy link
    Contributor

    Stale since PR #959

    @creatorrr creatorrr closed this Dec 31, 2024
    @creatorrr creatorrr reopened this Jan 16, 2025
    @creatorrr creatorrr marked this pull request as draft January 16, 2025 04:56
    @qodo-merge-pro-for-open-source Qodo Merge Pro for Open Source
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    791 - PR Code Verified

    Compliant requirements:

    • Enable lookup for labels in inputs and outputs
    • Support both numeric index and label-based lookup for inputs/outputs

    Requires further human verification:

    • Verify that YAML syntax like outputs["example"].a works as expected
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Data Validation

    The prepare_for_step method updates inputs/outputs dictionaries without validating if self.labels exists or is not None. This could lead to runtime errors.

    inputs = {i: input for i, input in enumerate(inputs)}
inputs.update(self.labels)

outputs = {i: output for i, output in enumerate(self.outputs)}
outputs.update(self.labels)
    State Management

    The workflow modifies the previous_labels dictionary directly when setting a new label. Consider creating a new dictionary to avoid potential side effects.

    if current_label:
    previous_labels[current_label] = final_output

    @qodo-merge-pro-for-open-source Qodo Merge Pro for Open Source
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Prevent dictionary key collisions between numeric indices and label names

    Add validation for dictionary keys in the inputs/outputs update to prevent potential
    key collisions between numeric indices and label names.

    agents-api/agents_api/common/protocol/tasks.py [253-254]

    -inputs = {i: input for i, input in enumerate(inputs)}
+inputs = {str(i): input for i, input in enumerate(inputs)}
+if any(str(i) in self.labels for i in range(len(inputs))):
+    raise ValueError("Label names conflict with numeric indices")
 inputs.update(self.labels)
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion addresses a critical potential bug where numeric indices could silently collide with label names, leading to data loss or incorrect behavior. The validation prevents such issues.

    		8
    Add type checking to prevent potential runtime errors when using dictionary keys

    Add validation to ensure current_label is not None before using it as a dictionary
    key, to prevent potential KeyError.

    agents-api/agents_api/workflows/task_execution/init.py [676-679]

     current_label = context.current_step.label
 
-if current_label:
+if current_label is not None and isinstance(current_label, str):
     previous_labels[current_label] = final_output
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: This is a valuable defensive programming suggestion that could prevent runtime errors by ensuring the label is both non-None and of the correct type before using it as a dictionary key.

    		7
    General
    Strengthen type safety by specifying allowed value types in dictionary

    Add type validation for the labels dictionary to ensure keys are strings and values
    are of compatible types. Consider using a more specific type annotation than Any.

    agents-api/agents_api/common/protocol/tasks.py [159]

    -labels: dict[str, Any] | RemoteObject
+labels: dict[str, Union[str, int, float, bool, dict, list]] | RemoteObject
    • Apply this suggestion
    Suggestion importance[1-10]: 3

    Why: While the suggestion improves type safety, it's a minor enhancement that might be too restrictive for the general use case. The current Any type is more flexible and common in Python codebases.

    		3

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @ellipsis-dev ellipsis-dev[bot] ellipsis-dev[bot] left review comments

    @creatorrr creatorrr Awaiting requested review from creatorrr

    Assignees
    No one assigned
    Labels
    Review effort [1-5]: 2
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.

    [Feature]: Add label lookup for inputs and outputs in tasks
    2 participants
    @Ahmad-mtos @creatorrr