Skip to content

Conversation

@sjrl
Copy link
Contributor

@sjrl sjrl commented Dec 3, 2025

Related Issues

Proposed Changes:

This PR updates components to auto run their warm_up method on first use. Pipelines already warm up components at runtime, so standalone use should follow the same pattern for a smoother developer experience.

How did you test it?

Updated existing tests

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 have added unit tests and updated the docstrings.
  • I've used one of the conventional commit types for my PR title: fix:, feat:, build:, chore:, ci:, docs:, style:, refactor:, perf:, test: and added ! in case the PR includes breaking changes.
  • I have documented my code.
  • I have added a release note file, following the contributors guidelines.
  • I have run pre-commit hooks and fixed any issue.

@sjrl sjrl requested a review from a team as a code owner December 3, 2025 08:17
@sjrl sjrl requested review from Amnah199 and removed request for a team December 3, 2025 08:18
@vercel
Copy link

vercel bot commented Dec 3, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Preview Comments Updated (UTC)
haystack-docs Ignored Ignored Preview Dec 10, 2025 8:28am

@github-actions github-actions bot added topic:tests type:documentation Improvements on the docs labels Dec 3, 2025
@davidsbatista davidsbatista requested review from davidsbatista and removed request for Amnah199 December 8, 2025 14:13
@davidsbatista
Copy link
Contributor

davidsbatista commented Dec 8, 2025

Seems like the test:

def test_raises_component_error_if_model_not_warmed_up(self):

in test/components/rankers/test_transformers_similarity.py can also be removed

It's a false positive - the test passes for the wrong reason, the model is being warmed up, but warm-up fails because HTTP is blocked I removed the line with pytest.raises(RuntimeError): to check for this

@davidsbatista
Copy link
Contributor

On another topic, I think assert statements should be avoided when they are used only to satisfy the type checker.

if self.model is None:
    self.warm_up()

# To make mypy happy even though this is set in warm_up()
assert self.model is not None

IMO, adding a # type:ignore and explaining in a short comment why it's being added is more suitable.

We are using a runtime safety tool (assert) to solve a type checker analysis problem (mypy limitations). I think those are different things that shouldn't be mixed.

Also, if the assumption is somehow broken, an AssertionError gives a more cryptic message compared to the actual error message. Also, and in a more far fetched use case, all assert statements can be disabled with the -O flag.

In any case I don't think the PR should be blocked by this, but I would be happy to hear what you think about it.

@sjrl
Copy link
Contributor Author

sjrl commented Dec 9, 2025

On another topic, I think assert statements should be avoided when they are used only to satisfy the type checker.

if self.model is None:
    self.warm_up()

# To make mypy happy even though this is set in warm_up()
assert self.model is not None

IMO, adding a # type:ignore and explaining in a short comment why it's being added is more suitable.

We are using a runtime safety tool (assert) to solve a type checker analysis problem (mypy limitations). I think those are different things that shouldn't be mixed.

Also, if the assumption is somehow broken, an AssertionError gives a more cryptic message compared to the actual error message. Also, and in a more far fetched use case, all assert statements can be disabled with the -O flag.

In any case I don't think the PR should be blocked by this, but I would be happy to hear what you think about it.

Yeah I was unsure how to proceed on this. For this PR I followed what we've done in other parts of the code base which was use assert statements, but I'd also be fine with using type ignore instead.

@anakin87 I'd appreciate your thoughts on this as well.

@anakin87
Copy link
Member

anakin87 commented Dec 9, 2025

Yeah I was unsure how to proceed on this. For this PR I followed what we've done in other parts of the code base which was use assert statements, but I'd also be fine with using type ignore instead.

@anakin87 I'd appreciate your thoughts on this as well.

It's a nuanced topic and I don't have strong opinions about. Sometimes having an assert raise a meaningful error feels helpful to me, but in this case I wouldn't expect it to ever be triggered.

I'm also OK with using a type: ignore[specific-error] here.

@sjrl
Copy link
Contributor Author

sjrl commented Dec 9, 2025

It's a nuanced topic and I don't have strong opinions about. Sometimes having an assert raise a meaningful error feels helpful to me, but in this case I wouldn't expect it to ever be triggered.

I'm also OK with using a type: ignore[specific-error] here.

Sounds good! I'll go ahead and update the PR to use type: ignore[specific-error] instead

@sjrl sjrl requested a review from davidsbatista December 10, 2025 08:24
@sjrl
Copy link
Contributor Author

sjrl commented Dec 10, 2025

@davidsbatista this should be ready for a final review, all added asserts should have been removed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

topic:tests type:documentation Improvements on the docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Instead of throwing a RuntimeError if a Component has not been warmed up just call warm_up at runtime

4 participants