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: Prevent component pre-init hook from being called "recursively" #7894

Merged

Conversation

shadeMe
Copy link
Collaborator

@shadeMe shadeMe commented Jun 19, 2024

Proposed Changes:

The pipeline deserialization callbacks make use of an internal hook to override the init parameters of components during the deserialization process. Prior to this PR, the hook would get invoked incorrectly under the following conditions:

  • The code path with the hook is taken.
  • The component's c'tor is invoked with the overridden init parameters.
  • Inside the c'tor, another component is directly instantiated.

This causes the hook to be invoked yet again but with the same callback, which is bound to the first component's name. We fix this by using an additional flag to detect if we're still executing the hook and exiting early.

How did you test it?

Unit test

Checklist

@shadeMe shadeMe added ignore-for-release-notes PRs with this flag won't be included in the release notes. 2.x Related to Haystack v2.0 topic:core labels Jun 19, 2024
@github-actions github-actions bot added topic:tests type:documentation Improvements on the docs labels Jun 19, 2024
@coveralls
Copy link
Collaborator

coveralls commented Jun 19, 2024

Pull Request Test Coverage Report for Build 9584502571

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.01%) to 90.164%

Files with Coverage Reduction New Missed Lines %
core/component/component.py 2 98.28%
Totals Coverage Status
Change from base Build 9583192592: 0.01%
Covered Lines: 7003
Relevant Lines: 7767

💛 - Coveralls

@shadeMe shadeMe marked this pull request as ready for review June 19, 2024 15:53
@shadeMe shadeMe requested a review from a team as a code owner June 19, 2024 15:53
@shadeMe shadeMe requested review from masci and removed request for a team June 19, 2024 15:53
@coveralls
Copy link
Collaborator

coveralls commented Jun 19, 2024

Pull Request Test Coverage Report for Build 9584993278

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.01%) to 90.164%

Files with Coverage Reduction New Missed Lines %
core/component/component.py 2 98.28%
Totals Coverage Status
Change from base Build 9583192592: 0.01%
Covered Lines: 7003
Relevant Lines: 7767

💛 - Coveralls

@masci masci requested review from silvanocerza and removed request for masci June 20, 2024 07:01
@shadeMe shadeMe merged commit d1f8c0d into deepset-ai:main Jun 21, 2024
28 checks passed
@shadeMe shadeMe deleted the fix/component-pre-init-hook-non-reentrant branch June 21, 2024 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.x Related to Haystack v2.0 ignore-for-release-notes PRs with this flag won't be included in the release notes. topic:core topic:tests type:documentation Improvements on the docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants