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: Run update_embeddings in examples #6008

Merged
merged 26 commits into from
Oct 10, 2023
Merged

fix: Run update_embeddings in examples #6008

merged 26 commits into from
Oct 10, 2023

Conversation

nickprock
Copy link
Contributor

@nickprock nickprock commented Oct 9, 2023

Proposed Changes:

  • Added document_store.update_embeddings to example pipelines for faq and hybrid faq
  • Skip PromptNode tests with external providers if API_KEY is not set. Previously, if you had no HUGGINGFACE_API_KEY set and then run test_getting_started , before this PR you would download falcon-7b-instruct.
  • Delete document indices after example pipelines ran. Otherwise faq pipeline reuses the document index from the qa example pipeline
  • Rename document index to example-document. Otherwise by running the example, users might delete their default index document with their own data
  • Install fewer dependencies, for example weaviate is not installed in the workflow because it's not needed, etc.
  • pylint no-logging-basicconfig check is disabled in examples
  • imports are sorted in examples
  • anthropic was missing in test_getting_started parameterized test

How did you test it?

manual verification, CI

Notes for the reviewer

Checklist

@nickprock nickprock requested review from a team as code owners October 9, 2023 13:34
@nickprock nickprock requested review from dfokina and julian-risch and removed request for a team October 9, 2023 13:34
Copy link
Member

@julian-risch julian-risch left a comment

Choose a reason for hiding this comment

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

Hi @nickprock Thanks for adding this fix and I see you already merged Haystack main just now. Before, the PR was still based on the version of July. 👍
Let's change the release notes so that the PR note is listed under fixes. We use upgrade for breaking changes with guidelines how to upgrade. Something like Added documents_store.update_embeddings to hybrid search example pipeline. under fixes would be great!

@julian-risch
Copy link
Member

@nickprock In the failing test you see that we also updated precommit hooks since last time.
pre-commit install should do the trick before committing. More details here: https://github.com/deepset-ai/haystack/blob/main/CONTRIBUTING.md#installation
Just let me know if you need help. 🖖

@coveralls
Copy link
Collaborator

coveralls commented Oct 9, 2023

Pull Request Test Coverage Report for Build 6470518728

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.004%) to 50.357%

Totals Coverage Status
Change from base Build 6469584398: 0.004%
Covered Lines: 12642
Relevant Lines: 25105

💛 - Coveralls

@nickprock
Copy link
Contributor Author

Hi @julian-risch I hope now it's correct

Copy link
Member

@julian-risch julian-risch 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! Thank you for this contribution. 👍 We should have noticed earlier ourselves but thanks to you it's fixed now. I'll merge the pull request as soon as all the test passed.

@julian-risch julian-risch changed the title added update_embeddings fix: Run update_embeddings in examples Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants