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

feat: Speed up integration tests (nodes) #3408

Merged
merged 3 commits into from
Oct 18, 2022
Merged

feat: Speed up integration tests (nodes) #3408

merged 3 commits into from
Oct 18, 2022

Conversation

sjrl
Copy link
Contributor

@sjrl sjrl commented Oct 18, 2022

Related Issues

  • No open issue

Proposed Changes:

  • Changed summarizer model to a smaller one (2GB to 500MB) to save on space and speed up the tests. Looking at the CI for integration-tests-linux(nodes) it looks like this change results in a large speedup (e.g. Run Tests goes from ~20min to ~8 min). Or another way to look at it is that the summarizer had 7/10 of the slowest tests before and now 0/10.

How did you test it?

Updated affected CI tests.

Notes for the reviewer

I noticed that the integration tests for nodes were taking awhile and I found that we could achieve an easy win in speedup by swapping out the model used for summarization.

Checklist

@sjrl sjrl changed the title feat: Exploring speeding up node tests feat: Sped up integration tests (nodes) Oct 18, 2022
@sjrl sjrl marked this pull request as ready for review October 18, 2022 10:14
@sjrl sjrl requested a review from a team as a code owner October 18, 2022 10:14
@sjrl sjrl requested review from masci and removed request for a team October 18, 2022 10:14
@sjrl sjrl changed the title feat: Sped up integration tests (nodes) feat: Speed up integration tests (nodes) Oct 18, 2022
@sjrl
Copy link
Contributor Author

sjrl commented Oct 18, 2022

Hey, @masci this PR is ready and I would appreciate a review when you have the chance 🙂

Copy link
Contributor

@masci masci left a comment

Choose a reason for hiding this comment

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

This is great, thanks!

Can you remove the caching for google/pegasus-xsum here? https://github.com/deepset-ai/haystack/blob/main/.github/workflows/tests.yml#L690?

@sjrl
Copy link
Contributor Author

sjrl commented Oct 18, 2022

@masci You're welcome! And definitely, I have removed the pegasus model from caching and I reactivated a test that wasn't run because its name did not follow pytest conventions.

Copy link
Contributor

@masci masci left a comment

Choose a reason for hiding this comment

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

LGTM, good catch with the test name!

@sjrl sjrl merged commit 93817f6 into main Oct 18, 2022
@sjrl sjrl deleted the sjrl-nodes-speed branch October 18, 2022 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants