Skip to content

Conversation

brandon-wada
Copy link
Collaborator

@brandon-wada brandon-wada commented Jun 4, 2024

Adds a few new convenience parameters when setting up a detector. Specifically, we want users to be able to specify the group_name and the patience time

Copy link
Contributor

@timmarkhuff timmarkhuff left a comment

Choose a reason for hiding this comment

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

LTGM. There was a CI failure, but once that's fixed, I would say it's good to merge.

I do worry a little bit that we raise exceptions when the group_name, confidence_threshold etc. are different than the existing detector. That sets the user up for a situation where their code works fine for a while, and then because of a change another user makes through the webapp, the code breaks. That said, I'm not sure what the right solution is. Perhaps:

  1. providing a value that differs from the existing detector's value updates that value
  2. don't include any of these parameters in this method. Have separate setter methods for these values (like how we already have one for setting the detector confidence)

Of the two options, I would probably vote for the first.

:param query: the detector query
:param group_name: the detector group that the new detector should belong to
Copy link
Contributor

@tyler-romero tyler-romero Jun 11, 2024

Choose a reason for hiding this comment

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

Should add what None defaults to, otherwise it seems like we wont set a group name if its not passed.

:param confidence_threshold: the confidence threshold
:param patience_time: the patience time, or how long Groundlight should work to generate a confident answer
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, what is the default?

@brandon-wada brandon-wada merged commit 2a62912 into main Aug 15, 2024
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