Skip to content

Conversation

mskarlin
Copy link
Collaborator

@mskarlin mskarlin commented Jul 1, 2025

We previously filtered these out when the embeddings, etc. were removed.

@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Jul 1, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the user-output filtering logic to preserve any extra fields on Context models and adds a test to verify that behavior.

  • Switch filter_content_for_user to use model_dump(exclude={"text"}), preserving all extras on Context
  • Add extra_field to a test context and assert it survives the filter

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
tests/test_agents.py Added extra_field when constructing a context and asserted it is retained after filtering
paperqa/types.py Replaced manual field listing with **c.model_dump(exclude={"text"}) to carry through extras
Comments suppressed due to low confidence (2)

paperqa/types.py:241

  • The docstring for filter_content_for_user says it filters out extra items, but the new implementation preserves extras. Please update the docstring to reflect that extras are now kept.
    def filter_content_for_user(self) -> None:

tests/test_agents.py:973

  • This assertion only checks that extra_field is non-null. Consider asserting the actual value (e.g., == "extra_value") to catch cases where the field is wrong or partially populated.
    assert response.session.contexts[0].extra_field is not None  # type: ignore[attr-defined]

@dosubot dosubot bot added the enhancement New feature or request label Jul 1, 2025
@mskarlin mskarlin requested a review from nadolskit July 1, 2025 22:56
@@ -969,6 +970,7 @@ def test_answers_are_striped() -> None:
assert not response.session.contexts[0].text.text
assert response.session.contexts[0].text.doc is not None
assert response.session.contexts[0].text.doc.embedding is None
assert response.session.contexts[0].extra_field is not None # type: ignore[attr-defined]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add an assertion message here as informal docs, something like , "Extra fields should be propagated so we can XYZ"

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jul 1, 2025
@mskarlin mskarlin merged commit cbccaee into main Jul 1, 2025
5 checks passed
@mskarlin mskarlin deleted the allow-extras-in-context branch July 1, 2025 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request lgtm This PR has been approved by a maintainer size:XS This PR changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants