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/add aisettings to django #927

Merged
merged 7 commits into from
Aug 7, 2024

Conversation

rachaelcodes
Copy link
Contributor

@rachaelcodes rachaelcodes commented Aug 7, 2024

Context

After #894 AISettings can be sent to core_api from Django as part of the chat request. This PR implements AISettings in Django.

Changes proposed in this pull request

  • Adds a new AISettings model with the same defaults as are currently in redbox-core (these will later be removed and kept only in Django)
  • Creates an instance of AISettings labelled default that is set as the default for all Django users
  • Communicates AISettings to core-api as part of the chat payload

Guidance to review

Try running locally and creating your own AISettings in the Django shell.

Relevant links

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

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

@rachaelcodes rachaelcodes marked this pull request as ready for review August 7, 2024 13:35
@rachaelcodes rachaelcodes requested a review from gecBurton August 7, 2024 13:37
@@ -219,6 +221,14 @@ def get_sources_with_files(
def file_save(file):
return file.save()

@staticmethod
@database_sync_to_async
def get_ai_settings(user: User) -> AISettings:
Copy link
Collaborator

Choose a reason for hiding this comment

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

this appears again in django_app/redbox_app/redbox_core/client.py ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't one for streamed and one for non-streamed requests?

model_name="user",
name="ai_settings",
field=models.ForeignKey(
default="default",
Copy link
Collaborator

Choose a reason for hiding this comment

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

TIL. nice

NewUser = new_state.apps.get_model("redbox_core", "User") # noqa: N806

for user in NewUser.objects.all():
assert user.ai_settings.label == "default"
Copy link
Collaborator

Choose a reason for hiding this comment

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

perfect

Copy link
Collaborator

@gecBurton gecBurton left a comment

Choose a reason for hiding this comment

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

once nitpick, otherwise great stuff

@rachaelcodes rachaelcodes force-pushed the feature/add-aisettings-to-django branch from ccae92c to e68cf0c Compare August 7, 2024 14:29
@rachaelcodes rachaelcodes merged commit 053d355 into main Aug 7, 2024
10 checks passed
@rachaelcodes rachaelcodes deleted the feature/add-aisettings-to-django branch August 7, 2024 14:46
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.

2 participants