-
Notifications
You must be signed in to change notification settings - Fork 150
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
verify feedback selectors on recorder init #961
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested.
- Reviewed the entire pull request up to 0cb3c5f
- Looked at
195
lines of code in2
files - Took 2 minutes and 51 seconds to review
More info
- Skipped
0
files when reviewing. - Skipped posting
0
additional comments because they didn't meet confidence threshold of50%
.
Workflow ID: wflow_KHxlrV4eb8P1Hk0F
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. We'll respond in a few minutes. Learn more here.
""" | ||
Check that the selectors are valid for the given app and record. | ||
""" | ||
return True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check_selectors
method currently does not perform any checks and always returns True. This could potentially lead to issues if the selectors are not valid for the given app and record. Please implement the necessary checks or remove the method if it's not needed.
@@ -415,6 +415,7 @@ | |||
higher_is_better=self.higher_is_better | |||
) | |||
|
|||
# alias | |||
on_input = on_prompt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The on_prompt
and on_input
methods are identical, as are on_response
and on_output
. Having two methods that do the same thing can lead to confusion. Consider removing one of each pair, or if they are needed for backward compatibility, mark one as deprecated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expecting this change to make a big difference in users understanding why they run into issues with the feedback selector. Nicely done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me!
- Performed an incremental review on c2289f5
- Looked at
5025
lines of code in41
files - Took 13 minutes and 30 seconds to review
More info
- Skipped
6
files when reviewing. - Skipped posting
6
additional comments because they didn't meet confidence threshold of50%
.
1. trulens_eval/trulens_eval/tru_chain.py:14
:
- Assessed confidence :
10%
- Comment:
TheTruChain
class is a wrapper for a Llama chain that adds logging and feedback functionality. It checks the selectors in the feedback functions and raises an error or prints a warning if a selector doesn't exist, depending on the settings. It also logs the calls to the__call__
method of thechain
and their results, and can be used as a context manager for automatic logging. - Reasoning:
TheTruChain
class is a wrapper for a Llama chain that adds logging and feedback functionality. The__init__
method takes achain
argument, which is the chain to be wrapped, and anapp_id
argument, which is a unique identifier for the app. It also takes afeedbacks
argument, which is a list of feedback functions to be used for evaluating the app. The__init__
method also has optional arguments for controlling the behavior of the wrapper, such asselectors_nocheck
andselectors_check_warning
, which control whether to check the selectors in the feedback functions and whether to raise an error if a selector doesn't exist. The__init__
method calls thecheck_selectors
method to check the selectors in the feedback functions. Thecheck_selectors
method raises aValueError
if a selector doesn't exist andselectors_nocheck
isFalse
. Ifselectors_check_warning
isTrue
, it prints a warning message instead of raising an error. TheTruChain
class also has a__call__
method that calls the__call__
method of thechain
and logs the call and its result. The__call__
method returns the result of the__call__
method of thechain
. The__call__
method also has awith_record
method that calls the__call__
method of thechain
and returns both the result and the record of the call. TheTruChain
class also has a__enter__
and a__exit__
method, which allow it to be used as a context manager. When used as a context manager, it automatically logs the calls to the__call__
method of thechain
and their results.
2. trulens_eval/trulens_eval/tru_llama.py:15
:
- Assessed confidence :
10%
- Comment:
TheTruLlama
class is a wrapper for a Llama Index app that adds logging and feedback functionality. It checks the selectors in the feedback functions and raises an error or prints a warning if a selector doesn't exist, depending on the settings. It also logs the calls to thequery
method of thequery_engine
and their results, and can be used as a context manager for automatic logging. - Reasoning:
TheTruLlama
class is a wrapper for a Llama Index app that adds logging and feedback functionality. The__init__
method takes aquery_engine
argument, which is the query engine of the Llama Index app to be wrapped, and anapp_id
argument, which is a unique identifier for the app. It also takes afeedbacks
argument, which is a list of feedback functions to be used for evaluating the app. The__init__
method also has optional arguments for controlling the behavior of the wrapper, such asselectors_nocheck
andselectors_check_warning
, which control whether to check the selectors in the feedback functions and whether to raise an error if a selector doesn't exist. The__init__
method calls thecheck_selectors
method to check the selectors in the feedback functions. Thecheck_selectors
method raises aValueError
if a selector doesn't exist andselectors_nocheck
isFalse
. Ifselectors_check_warning
isTrue
, it prints a warning message instead of raising an error. TheTruLlama
class also has a__call__
method that calls thequery
method of thequery_engine
and logs the call and its result. The__call__
method returns the result of thequery
method of thequery_engine
. The__call__
method also has awith_record
method that calls thequery
method of thequery_engine
and returns both the result and the record of the call. TheTruLlama
class also has a__enter__
and a__exit__
method, which allow it to be used as a context manager. When used as a context manager, it automatically logs the calls to thequery
method of thequery_engine
and their results.
3. trulens_eval/trulens_eval/tru_rails.py:15
:
- Assessed confidence :
10%
- Comment:
TheTruRails
class is a wrapper for a NEMO Guardrails app that adds logging and feedback functionality. It checks the selectors in the feedback functions and raises an error or prints a warning if a selector doesn't exist, depending on the settings. It also logs the calls to the__call__
method of theapp
and their results, and can be used as a context manager for automatic logging. - Reasoning:
TheTruRails
class is a wrapper for a NEMO Guardrails app that adds logging and feedback functionality. The__init__
method takes anapp
argument, which is the app to be wrapped, and anapp_id
argument, which is a unique identifier for the app. It also takes afeedbacks
argument, which is a list of feedback functions to be used for evaluating the app. The__init__
method also has optional arguments for controlling the behavior of the wrapper, such asselectors_nocheck
andselectors_check_warning
, which control whether to check the selectors in the feedback functions and whether to raise an error if a selector doesn't exist. The__init__
method calls thecheck_selectors
method to check the selectors in the feedback functions. Thecheck_selectors
method raises aValueError
if a selector doesn't exist andselectors_nocheck
isFalse
. Ifselectors_check_warning
isTrue
, it prints a warning message instead of raising an error. TheTruRails
class also has a__call__
method that calls the__call__
method of theapp
and logs the call and its result. The__call__
method returns the result of the__call__
method of theapp
. The__call__
method also has awith_record
method that calls the__call__
method of theapp
and returns both the result and the record of the call. TheTruRails
class also has a__enter__
and a__exit__
method, which allow it to be used as a context manager. When used as a context manager, it automatically logs the calls to the__call__
method of theapp
and their results.
4. trulens_eval/trulens_eval/tru_rails.py:169
:
- Assessed confidence :
10%
- Comment:
TheRailsInstrument
class is a wrapper for a NEMO Guardrails app that adds logging functionality. It checks the selectors in the feedback functions and raises an error or prints a warning if a selector doesn't exist, depending on the settings. It also logs the calls to the__call__
method of theapp
and their results, and can be used as a context manager for automatic logging. - Reasoning:
TheRailsInstrument
class is a wrapper for a NEMO Guardrails app that adds logging functionality. The__init__
method takes anapp
argument, which is the app to be wrapped, and anapp_id
argument, which is a unique identifier for the app. The__init__
method also has optional arguments for controlling the behavior of the wrapper, such asselectors_nocheck
andselectors_check_warning
, which control whether to check the selectors in the feedback functions and whether to raise an error if a selector doesn't exist. The__init__
method calls thecheck_selectors
method to check the selectors in the feedback functions. Thecheck_selectors
method raises aValueError
if a selector doesn't exist andselectors_nocheck
isFalse
. Ifselectors_check_warning
isTrue
, it prints a warning message instead of raising an error. TheRailsInstrument
class also has a__call__
method that calls the__call__
method of theapp
and logs the call and its result. The__call__
method returns the result of the__call__
method of theapp
. The__call__
method also has awith_record
method that calls the__call__
method of theapp
and returns both the result and the record of the call. TheRailsInstrument
class also has a__enter__
and a__exit__
method, which allow it to be used as a context manager. When used as a context manager, it automatically logs the calls to the__call__
method of theapp
and their results.
5. trulens_eval/trulens_eval/tru_llama.py:28
:
- Assessed confidence :
10%
- Comment:
Thecheck_selectors
method checks the selectors in the feedback functions against the app and the record. If a selector doesn't exist andselectors_nocheck
isFalse
, it raises aValueError
. Ifselectors_check_warning
isTrue
, it prints a warning message instead of raising an error. The error message includes a dump of the longest prefix of the selector that does exist. - Reasoning:
Thecheck_selectors
method checks the selectors in the feedback functions against the app and the record. If a selector doesn't exist andselectors_nocheck
isFalse
, it raises aValueError
. Ifselectors_check_warning
isTrue
, it prints a warning message instead of raising an error. The error message includes a dump of the longest prefix of the selector that does exist. This method is used in the__init__
method of theTruLlama
andTruRails
classes to check the selectors in the feedback functions when the classes are instantiated.
6. trulens_eval/trulens_eval/utils/notebook_utils.py:51
:
- Assessed confidence :
90%
- Grade:
0%
- Comment:
Thiswritefileinterpolated
function is not used anywhere in the codebase. Consider removing it if it's not needed. Also, it's generally a good practice to avoid defining functions conditionally as it can lead to unexpected behavior. - Reasoning:
The functionwritefileinterpolated
is not used anywhere in the codebase. It seems to be a utility function for writing to a file, but it's not clear why it's needed in this context. It's also not clear why it's conditionally defined based on the result ofis_notebook()
. This could potentially lead to unexpected behavior if the code is run in different environments.
Workflow ID: wflow_XXNjdE2UlsuaAvIM
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
…dummy records to check against
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
something_referring_to_method_m.args.method_m_arg.anything
orsomething_referring_to_method_m.rets.anything
as info beyond known parameter names or return values is not known before app is invoked.App.dummy_record
method to produce the records used to check for selector issue but might be independently useful for users.When creating an app recorder and feedbacks are provided, the selectors in those feedbacks are checked against the app and (empty) record in case those selectors are wrong. Settings to not run this check or not throw the error are provided and explained in the error message. The message also includes a dump of the longest prefix of the selector that does exist. Example:
Produces an exception and a hint message:
Summary:
This PR introduces a method to verify feedback selectors, improves comments and logging, adds support for NEMO Guardrails apps with new classes, and updates test files and documentation.
Key points:
check_selectors
inApp
andFeedback
classes inapp.py
andfeedback.py
respectively.app.py
andfeedback.py
.TruRails
andRailsInstrument
classes intru_rails.py
.Generated with ❤️ by ellipsis.dev
Summary:
This PR maintains the detailed explanation of the error handling mechanism, introduces a method to verify feedback selectors, improves comments and logging, adds support for NEMO Guardrails apps, and updates test files and documentation.
Key points:
app.py
andfeedback.py
.TruRails
andRailsInstrument
classes intru_rails.py
.Generated with ❤️ by ellipsis.dev