Fix failing test #195
Fix failing test #195
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested.
- Reviewed the entire pull request up to 3e6ce60
- Looked at
1271lines of code in4files - Took 3 minutes and 47 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
3additional comments because they didn't meet confidence threshold of50%.
1. edsl/data/Cache.py:684:
- Assessed confidence :
100% - Grade:
30% - Comment:
The__eq__method should also check if the corresponding values of the keys are equal. Consider changing it to:
return self.data.keys() == other_cache.data.keys() and all(self.data[key] == other_cache.data[key] for key in self.data.keys())
- Reasoning:
In the Cache.py file, the__eq__method of the Cache class is only checking if the keys of the data dictionaries of the two Cache instances are equal. It should also check if the corresponding values (CacheEntry objects) are equal.
2. edsl/data/CacheEntry.py:103:
- Assessed confidence :
100% - Grade:
0% - Comment:
The__eq__method should check if theotherobject is an instance of CacheEntry before accessing its attributes to avoid an AttributeError.
if not isinstance(other, CacheEntry):
return False
return all(getattr(self, field) == getattr(other, field) for field in self.all_fields if field != 'timestamp')
- Reasoning:
In the CacheEntry.py file, the__eq__method of the CacheEntry class is not checking if theotherobject is an instance of CacheEntry before accessing its attributes. This could lead to an AttributeError if theotherobject is not an instance of CacheEntry.
3. edsl/data/CacheEntry.py:165:
- Assessed confidence :
100% - Grade:
0% - Comment:
Thestore_input_examplemethod should also remove the 'output' key from the dictionary to avoid confusion.
input = cls.example().to_dict()
_ = input.pop('timestamp')
_ = input.pop('output')
input['response'] = input.pop('output')
return input
- Reasoning:
In the CacheEntry.py file, thestore_input_examplemethod of the CacheEntry class is popping the 'timestamp' key from the input dictionary and adding a 'response' key. However, it is not removing the 'output' key. This could lead to confusion as both 'response' and 'output' keys would be present in the dictionary.
Workflow ID: wflow_DO63dc26mgGEi1sU
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.
| return False | ||
| return self.data.keys() == other_cache.data.keys() | ||
|
|
||
| def __add__(self, other: "Cache"): |
There was a problem hiding this comment.
The __add__ method is modifying the current instance. It should return a new instance with the combined data. Consider changing it to:
| def __add__(self, other: "Cache"): | |
| new_cache = Cache() | |
| new_cache.data = self.data.copy() | |
| new_cache.data.update(other.data) | |
| return new_cache |
There was a problem hiding this comment.
This is an interesting suggestion, @ellipsis, but this would make a new Cache w/o a sqlite backend. We need to think about this one more. Might just not allow __ add __ here
There was a problem hiding this comment.
I was in the process of addressing the reviewer's comment about the __add__ method in the Cache class. The reviewer suggested that this method might not be necessary in this context. Therefore, I was planning to remove this method from the class. However, I ran out of time before I could make this change. I would appreciate any feedback or suggestions on how to proceed. Thank you!
Responding to this comment by @johnjosephhorton. For more information about Ellipsis, check the documentation.
There was a problem hiding this comment.
👍 Looks good to me!
- Performed an incremental review on de65a5b
- Looked at
13lines of code in1files - Took 50 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
1additional comments because they didn't meet confidence threshold of50%.
1. tests/language_models/test_LanguageModel.py:129:
- Assessed confidence :
0% - Comment:
This change seems to be fixing a test case where the 'parameters' attribute was previously being compared as a string, but now it's being compared as a dictionary. This change seems to be in line with the PR description where it was mentioned that the 'store' command for cache entry was converting parameters to string, which was causing issues. I don't see any obvious issues with this change. - Reasoning:
The change in the test file seems to be fixing a test case where the 'parameters' attribute was previously being compared as a string, but now it's being compared as a dictionary. This change seems to be in line with the PR description where it was mentioned that the 'store' command for cache entry was converting parameters to string, which was causing issues. I don't see any obvious issues with this change.
Workflow ID: wflow_KFPlfAiFQ98EJbyB
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
There was a problem hiding this comment.
❌ Changes requested.
- Performed an incremental review on b36628e
- Looked at
41lines of code in2files - Took 1 minute and 48 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
1additional comments because they didn't meet confidence threshold of50%.
1. tests/data/test_CacheEntry.py:49:
- Assessed confidence :
0% - Comment:
The test cases have been updated correctly to reflect the changes in the 'gen_key' method. - Reasoning:
The test cases in 'test_CacheEntry.py' have been updated to reflect the changes in the 'gen_key' method. The expected keys in 'test_CacheEntry_gen_key' and 'test_CacheEntry_key_property' have been updated correctly. No issues found here.
Workflow ID: wflow_BxqsPi91EezeyINQ
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.
| Generates a key for the cache entry. | ||
| - Treats single and double quotes as the same. TODO: add more robustness. | ||
| """ | ||
| #long_key = f"{model}{parameters}{system_prompt}{user_prompt}{iteration}" |
There was a problem hiding this comment.
Please remove the commented out line of code for better code readability.
|
Ok @apostolosfilippas this should fix that issue you had. It does surface a bigger issue we have in the unit testing, re: failures caused by "simple"bugs that only show up in the Interview test in this fairly cryptic way |
There were a few issues:
@ellipsis - can you summarize code changes?
Summary:
This PR addresses several issues, makes improvements to the
CacheandCacheEntryclasses, updates corresponding test files, and modifies theCacheEntryclass to correctly handle parameters in the cache entry key generation.Key points:
CacheandCacheEntryclasses by adding type hints, improving docstrings, refactoring some methods.test_CacheEntry.pyandtest_LanguageModel.pyto reflect changes in the main codebase.gen_keymethod inCacheEntryclass to convertparametersto a JSON string before generating the key.test_CacheEntry_gen_keyandtest_CacheEntry_key_propertytests to reflect the change in key generation.Generated with ❤️ by ellipsis.dev