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

fix(agents-api): Fix remote blob store loading before transitions #696

Merged
merged 3 commits into from
Oct 18, 2024

Conversation

creatorrr
Copy link
Contributor

@creatorrr creatorrr commented Oct 18, 2024

Signed-off-by: Diwank Singh Tomer [email protected]


Important

Fixes remote blob store loading in transition_step and JSON serialization in create_execution_transition.

  • Behavior:
    • transition_step in transition_step.py now loads output from blob store if remote using load_from_blob_store_if_remote().
    • create_execution_transition in create_execution_transition.py serializes data.output to JSON if it has model_dump.
  • Code Cleanup:
    • Removed redundant model_dump function in task_execution/__init__.py.
  • Misc:
    • Added notebooks/Untitled*.ipynb to .gitignore.

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

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.

👍 Looks good to me! Reviewed everything up to 6ba02be in 16 seconds

More details
  • Looked at 101 lines of code in 5 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. agents-api/agents_api/activities/task_steps/transition_step.py:17
  • Draft comment:
    Consider checking if transition_info.output is remote before calling load_from_blob_store_if_remote to avoid unnecessary calls.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The function load_from_blob_store_if_remote is used to load data from a remote blob store if the data is remote. However, the function is called without checking if the data is indeed remote. This could lead to unnecessary calls to the blob store, which could be a performance issue.
2. agents-api/agents_api/models/execution/create_execution_transition.py:98
  • Draft comment:
    The JSON serialization logic for data.output is duplicated in TaskExecutionWorkflow.run. Consider centralizing this logic to avoid duplication.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code in create_execution_transition is responsible for serializing the data.output to JSON format. However, this logic is duplicated in the run function of TaskExecutionWorkflow. The duplication should be removed to avoid inconsistencies and maintenance issues.
3. agents-api/agents_api/workflows/task_execution/__init__.py:532
  • Draft comment:
    The model_dump logic was removed but is still needed for JSON serialization. Consider centralizing this logic to avoid duplication and potential errors.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The model_dump function was removed from the run function in TaskExecutionWorkflow, but the logic for converting objects to JSON is still needed. This logic should be centralized to avoid duplication and potential errors.

Workflow ID: wflow_ULFHzEeeF162mGUi


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

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.

👍 Looks good to me! Incremental review on 6ea7aa0 in 39 seconds

More details
  • Looked at 195 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. agents-api/agents_api/common/storage_handler.py:55
  • Draft comment:
    The deep parameter in auto_blob_store is not used effectively. The current logic overwrites new_args and new_kwargs without utilizing the deep parameter correctly. Consider revising the logic to ensure deep has the intended effect.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.

Workflow ID: wflow_P2VwtZkBJpQomDwX


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@Vedantsahai18 Vedantsahai18 merged commit ee24b86 into dev Oct 18, 2024
6 checks passed
@Vedantsahai18 Vedantsahai18 deleted the x/fix-remote-blob-load branch October 18, 2024 19:57
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