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(haystack): add input and output #1202

Merged
merged 5 commits into from
Jun 7, 2024

Conversation

tibor-reiss
Copy link
Contributor

@tibor-reiss tibor-reiss commented Jun 2, 2024

Towards #1169

  • I have added tests that cover my changes.
  • If adding a new instrumentation or changing an existing one, I've added screenshots from some observability platform showing the change.
  • PR name follows conventional commits format: feat(instrumentation): ... or fix(instrumentation): ....
  • (If applicable) I have updated the documentation accordingly.

image

@tibor-reiss tibor-reiss changed the title Add input and output fix(haystack): add input and output Jun 2, 2024
@nirga
Copy link
Member

nirga commented Jun 6, 2024

@tibor-reiss want to rebase and fix the lint issues? we can merge this one as well (although it won't solve #1169 completely - it's a much needed work in this direction)

@tibor-reiss tibor-reiss force-pushed the 1169-haystack-input-output branch 2 times, most recently from 3842155 to 7837e22 Compare June 6, 2024 18:44
@tibor-reiss
Copy link
Contributor Author

@tibor-reiss want to rebase and fix the lint issues? we can merge this one as well (although it won't solve #1169 completely - it's a much needed work in this direction)

@nirga Rebased, fixed linting issue.

kwargs_to_serialize[key] = value
args_to_serialize = [arg for arg in args if not isinstance(arg, dict)]
input_entity = {"args": args_to_serialize, "kwargs": kwargs_to_serialize}
return json.dumps(input_entity, cls=EnhancedJSONEncoder)
Copy link
Member

Choose a reason for hiding this comment

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

@tibor-reiss almost missed this - should be behind if should_send_prompts()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks!

Copy link
Contributor Author

@tibor-reiss tibor-reiss Jun 6, 2024

Choose a reason for hiding this comment

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

@nirga Just looked at it a second time: should this check rather be in wrap_pipeline.py? Otherwise both spans will be set to None even if should_send_prompts would be False.
I was using the implementation in langchain as blueprint, however I did not like that much that the process_request / process_response functions change state (it's kinda a side effect, and also harder to test).
Let me know your preference!

Copy link
Member

Choose a reason for hiding this comment

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

I'd keep it separate since we may be adding more things to process_request / process_response

@tibor-reiss tibor-reiss force-pushed the 1169-haystack-input-output branch from 920e41e to b5f7825 Compare June 7, 2024 17:45
Copy link
Member

@nirga nirga left a comment

Choose a reason for hiding this comment

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

Thank you @tibor-reiss!

@nirga nirga merged commit 09fb766 into traceloop:main Jun 7, 2024
8 checks passed
@tibor-reiss tibor-reiss deleted the 1169-haystack-input-output branch June 7, 2024 18:16
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