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

fix: extend schema for prompt node results #3891

Merged
merged 9 commits into from
Jan 31, 2023
Merged

fix: extend schema for prompt node results #3891

merged 9 commits into from
Jan 31, 2023

Conversation

tstadel
Copy link
Member

@tstadel tstadel commented Jan 19, 2023

Related Issues

Proposed Changes:

  • add "results" to QueryResponse schema

How did you test it?

  • added unit test

Notes for the reviewer

Checklist

  • I have read the contributors guidelines and the code of conduct
  • I have updated the related issue with new insights and changes
  • I added tests that demonstrate the correct behavior of the change
  • I've used one of the conventional commit types for my PR title: fix:, feat:, build:, chore:, ci:, docs:, style:, refactor:, perf:, test:.
  • I documented my code
  • I ran pre-commit hooks and fixed any issue

@tstadel tstadel requested a review from a team as a code owner January 19, 2023 13:08
@tstadel tstadel requested review from vblagoje and removed request for a team January 19, 2023 13:08
bogdankostic
bogdankostic approved these changes Jan 30, 2023
View reviewed changes
Copy link
Contributor

@bogdankostic bogdankostic left a comment

Choose a reason for hiding this comment

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

LGTM

@mayankjobanputra
Copy link
Contributor

@bogdankostic don't you think we also will have to manually change or run the script to make this change available to 1.14 rc also because 1.13 rc is already done I guess? I am not sure though.

bogdankostic
bogdankostic suggested changes Jan 30, 2023
View reviewed changes
Copy link
Contributor

@bogdankostic bogdankostic left a comment

Choose a reason for hiding this comment

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

@mayankjobanputra is right, we need to update the specs for 1.14.0rc0.

@sjrl
Copy link
Contributor

sjrl commented Jan 30, 2023

@mayankjobanputra @bogdankostic I thought we wanted to include this change into 1.13 as well? (asking since we removed the 1.13 schema changes) Or are we waiting until Haystack 1.14 before PromptNode is supported in the Rest-API?

@mayankjobanputra
Copy link
Contributor

@sjrl 1.13-rc has been out for almost a week now. The final 1.13 release should be out in less than 48 hrs if I am not wrong.

Adding this to 1.13, I am not at all against it, but my concerns are

  1. this would increase Aga's work (@agnieszka-m)
  2. testing the REST API before we publish 1.13 public release (I am sure @tstadel has tested this manually as well)

I think the release manager (who's @ZanSara for this release) should decide if we should take this for 1.13 or not.

@ZanSara
Copy link
Contributor

ZanSara commented Jan 31, 2023

Hey @sjrl, I agree with @mayankjobanputra here: it's quite late for this one to be included in 1.13, as the rc has been out for some time and this is not a critical bugfix.

However, if you get this one merged in the next few days, I might consider a patch release with some more features that arrived too late for 1.13, so I'll update the milestone to keep track of this.

@ZanSara ZanSara added this to the 1.13.1 milestone Jan 31, 2023
@ZanSara ZanSara added type:feature New feature or request topic:LLM labels Jan 31, 2023
bogdankostic
bogdankostic approved these changes Jan 31, 2023
View reviewed changes
Copy link
Contributor

@bogdankostic bogdankostic left a comment

Choose a reason for hiding this comment

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

Looking good to me now. Thanks @mayankjobanputra :)

@sjrl sjrl merged commit 8002cf9 into main Jan 31, 2023
@sjrl sjrl deleted the fix/rest_api_for_promptnode branch January 31, 2023 15:31
ZanSara added a commit that referenced this pull request Feb 2, 2023
@tstadel @bogdankostic
@mayankjobanputra @sjrl @ZanSara
fix: extend schema for prompt node results (#3891)
e0f3b7d 
* extend schema for prompt node results

* extend schema

* update openapi

* fix mypy for test module

* added 1.14 specs

* reverted schema for 1.13

---------

Co-authored-by: bogdankostic <[email protected]>
Co-authored-by: Mayank Jobanputra <[email protected]>
Co-authored-by: Sebastian <[email protected]>
Co-authored-by: ZanSara <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bogdankostic bogdankostic bogdankostic approved these changes

@vblagoje vblagoje Awaiting requested review from vblagoje vblagoje is a code owner automatically assigned from deepset-ai/open-source-engineering

Assignees
No one assigned
Labels
topic:LLM topic:rest_api type:feature New feature or request
Projects
None yet
Milestone
1.13.1
Development

Successfully merging this pull request may close these issues.

PromptNode's results are not exposed via rest_api
5 participants
@tstadel @mayankjobanputra @sjrl @ZanSara @bogdankostic