Guardrails request config validation #371
Conversation
mdevino
force-pushed
the
guardrails_validation_3
branch
2 times, most recently
from
3f34002 to
b417ca2
Compare
src/utils.rs
Outdated
| @@ -15,3 +25,43 @@ impl AsUriExt for Url { | |||
| Uri::try_from(self.to_string()).unwrap() | |||
| } | |||
| } | |||
|
|
|||
| /// Validates guardrails on request. | |||
| pub fn validate_guardrails( | |||
There was a problem hiding this comment.
Actually, let's just name this validate_detectors since it aligns better with what it does. Can you move it to orchestrator/common/utils.rs?
src/utils.rs
Outdated
| pub fn validate_guardrails( | ||
| request_detectors: &HashMap<String, DetectorParams>, | ||
| orchestrator_detectors: &HashMap<String, DetectorConfig>, | ||
| allowed_detector_types: Vec<DetectorType>, |
There was a problem hiding this comment.
nit: allowed_detector_types should be a slice instead of a Vec
There was a problem hiding this comment.
Thanks. I knew Vec seemed unnecessarly, but couldn't figure out a better way of doing it.
src/utils.rs
Outdated
| ) -> Result<(), Error> { | ||
| let whole_doc_chunker_id = DEFAULT_CHUNKER_ID; | ||
| let detector_ids = request_detectors.keys(); | ||
| for detector_id in detector_ids { |
There was a problem hiding this comment.
nit: just iterate over request_detectors.keys() directly instead of assigning to a variable. I would also rename request_detectors to simply detectors.
mdevino
force-pushed
the
guardrails_validation_3
branch
from
572bdfe to
13dbb4b
Compare
Nice catch! Fixed. |
|
Hmm, does this need to be rebased? |
It doesn't seem so:
I started working on this when you were halfway through #355, so I had to rebase at some point and resolve conflicts. That's when the extra commits showed up. |
Signed-off-by: Mateus Devino <[email protected]>
Signed-off-by: Mateus Devino <[email protected]>
Signed-off-by: Mateus Devino <[email protected]>
Signed-off-by: Mateus Devino <[email protected]>
Signed-off-by: Mateus Devino <[email protected]>
Signed-off-by: Mateus Devino <[email protected]>
Signed-off-by: Mateus Devino <[email protected]>
Signed-off-by: Mateus Devino <[email protected]>
Signed-off-by: Mateus Devino <[email protected]>
Signed-off-by: Mateus Devino <[email protected]>
Signed-off-by: Mateus Devino <[email protected]>
Signed-off-by: Mateus Devino <[email protected]>
Signed-off-by: Mateus Devino <[email protected]>
Co-authored-by: Dan Clark <[email protected]> Signed-off-by: Mateus Devino <[email protected]>
Co-authored-by: Dan Clark <[email protected]> Signed-off-by: Mateus Devino <[email protected]>
Co-authored-by: Dan Clark <[email protected]> Signed-off-by: Mateus Devino <[email protected]>
Co-authored-by: Dan Clark <[email protected]> Signed-off-by: Mateus Devino <[email protected]>
Co-authored-by: Dan Clark <[email protected]> Signed-off-by: Mateus Devino <[email protected]>
Signed-off-by: Mateus Devino <[email protected]>
Signed-off-by: Mateus Devino <[email protected]>
mdevino
force-pushed
the
guardrails_validation_3
branch
from
13dbb4b to
863d033
Compare
| // input detectors validation | ||
| if let Err(error) = validate_detectors( | ||
| &input_detectors, | ||
| &ctx.config.detectors, | ||
| &[DetectorType::TextContents], | ||
| false, | ||
| ) { | ||
| let _ = response_tx.send(Err(error)).await; | ||
| return; | ||
| } | ||
|
|
||
| // output detectors validation | ||
| if let Err(error) = validate_detectors( | ||
| &output_detectors, | ||
| &ctx.config.detectors, | ||
| &[DetectorType::TextContents], | ||
| false, | ||
| ) { | ||
| let _ = response_tx.send(Err(error)).await; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Since a user issue led to triggering of an error here for the current version of the orchestrator - why are the whole doc chunkers deliberately disallowed here, especially for both input and output streaming? This would disallow a case of say using Granite Guardian as a content detector.
Closes #352, and also adds appropriate integration tests.