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

Use new core schemas in guardrails API #46

Merged
merged 25 commits into from
Jun 14, 2024
Merged

Conversation

nichwch
Copy link
Contributor

@nichwch nichwch commented Jun 11, 2024

There are a few outstanding questions about the code I'm not sure about:

  1. It looks like the new IGuard interface doesn't have num_reasks. How do we populate that field in a GuardItem or postgres row? Reading through comments in the OSS Guard class it looks like num_reasks is now something you pass in when calling the guard. Would it be cleaner to stop storing reasks in Postgres/GuardItems as well then?
  2. I assume it's safe to populate the railspec column in GuardItem/Postgres with a dict of a guard, and that the main functionality of that column is to store a serialized guard. I wonder if it might be cleaner to rename that column because as far as I know RAIL is XML, not whatever we're sticking in now. I don't know if that requires a migration or anything, or if it's too much work to justify.

@nichwch nichwch requested a review from CalebCourier June 11, 2024 22:18
@CalebCourier
Copy link
Contributor

There are a few outstanding questions about the code I'm not sure about:

  1. It looks like the new IGuard interface doesn't have num_reasks. How do we populate that field in a GuardItem or postgres row? Reading through comments in the OSS Guard class it looks like num_reasks is now something you pass in when calling the guard. Would it be cleaner to stop storing reasks in Postgres/GuardItems as well then?
  2. I assume it's safe to populate the railspec column in GuardItem/Postgres with a dict of a guard, and that the main functionality of that column is to store a serialized guard. I wonder if it might be cleaner to rename that column because as far as I know RAIL is XML, not whatever we're sticking in now. I don't know if that requires a migration or anything, or if it's too much work to justify.
  1. Yes, that's becoming runtime/execution time only. I think we can stop storing it in postgres unless @zsimjee disagrees/has a use case to keep it around.
  2. I think we just need to go back to the drawing board on the database schema in general. For now, storing the entire guard in the rail column is probably fine until we figure out what we want to do databasewise, just make sure to update the postgres client accordingly. The reason we were using postgres in the first place was for it's vector store support for an ingestion service that's no longer in scope. We'll probably end up moving to a key-value or document store pretty soon, or at least using postgres like a document store via jsonb columns.

@CalebCourier CalebCourier changed the base branch from main to feat/streaming-and-in-mem June 13, 2024 15:42
Base automatically changed from feat/streaming-and-in-mem to main June 13, 2024 16:50
@CalebCourier CalebCourier marked this pull request as ready for review June 14, 2024 14:29
@CalebCourier CalebCourier merged commit 1cd5139 into main Jun 14, 2024
1 check passed
@CalebCourier CalebCourier deleted the nichwch/core-schema-impl branch June 14, 2024 14:29
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