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

Feature/redbox 367 show citation text #696

Merged
merged 11 commits into from
Jul 1, 2024

Conversation

brunns
Copy link
Contributor

@brunns brunns commented Jul 1, 2024

Context

Allow the user to see some detail of the citations.

Changes proposed in this pull request

Screenshot 2024-07-01 at 13 12 38

Screenshot 2024-07-01 at 13 12 48

Guidance to review

Relevant links

https://technologyprogramme.atlassian.net/browse/REDBOX-367

Things to check

  • I have added any new ENV vars in all deployed environments
  • I have tested any code added or changed
  • I have run integration tests

@@ -209,6 +209,9 @@ def expires_at(self) -> datetime:
def expires(self) -> timedelta:
return self.expires_at - datetime.now(tz=UTC)

def __lt__(self, other):
return self.id < other.id
Copy link
Contributor

Choose a reason for hiding this comment

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

Would created_at be a better metric to sort by? Or does the order not matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now, it doesn't matter - we only need a total ordering so that we can group citations by file. If one day we need some specific order, we can revisit.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've put in another comment about the grouping by file - can we preserve the order of the 'most relevant file'? Or is that for a later PR?

Copy link
Contributor Author

@brunns brunns Jul 1, 2024

Choose a reason for hiding this comment

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

Right now we don't store the citation order in the DB at all AFAICT. We might need to revisit that, because while streaming chat shows them in the order they came in, revisiting the chats page might show them in any order, even before this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think order_by("created_at") for Citations could do this, but it might need some more testing.

Copy link
Contributor

@rachaelcodes rachaelcodes left a comment

Choose a reason for hiding this comment

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

One problem of this approach is that the order of the source responses is lost on the citations page.

e.g. on the chat view - which has the most relevant source file listed first:
chat_sources

and in the citations page view - which loses this ordering
sources_with_citations

Given that this is the 'quick' initial round of showing citations, I would be happy with this discrepancy if Charlotte and Gabe are.

Copy link
Contributor

@rachaelcodes rachaelcodes left a comment

Choose a reason for hiding this comment

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

A good quick pass at this, which I think will add a lot of user value. Approved pending approval from Gabe/Charlotte re the discrepancy in File order.

@brunns brunns merged commit 3bdb79b into main Jul 1, 2024
8 checks passed
@brunns brunns deleted the feature/REDBOX-367-show-citation-text branch July 1, 2024 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants