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

Fixes cache issue from 703 and 679 #707

Merged
merged 7 commits into from
Nov 20, 2023
Merged

Fixes cache issue from 703 and 679 #707

merged 7 commits into from
Nov 20, 2023

Conversation

afourney
Copy link
Member

Why are these changes needed?

The cache database is created even when caching is disabled. This raises issues on read-only filesystems, and is also generally unpleasant, as it leaves debris everywhere it is run. This PR fixed the issue.

Related issue number

Closes #703 and #679

Checks

@afourney
Copy link
Member Author

@sonichi While I was in there, fixing cache, I noticed that the filter logic only ever applied to cached responses. Is this what was intended? Or is that a separate bug?

Copy link
Contributor

@qingyun-wu qingyun-wu left a comment

Choose a reason for hiding this comment

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

LGTM, thanks Adam!

Copy link
Contributor

@sonichi sonichi left a comment

Choose a reason for hiding this comment

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

You are right. There are some bugs about the filter check.

autogen/oai/client.py Show resolved Hide resolved
autogen/oai/client.py Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Nov 20, 2023

Codecov Report

Attention: Patch coverage is 43.75000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 49.08%. Comparing base (a4d9ce8) to head (3c178ea).
Report is 1662 commits behind head on main.

Files with missing lines Patch % Lines
autogen/oai/client.py 43.75% 6 Missing and 3 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #707       +/-   ##
===========================================
+ Coverage   29.81%   49.08%   +19.27%     
===========================================
  Files          27       27               
  Lines        3448     3455        +7     
  Branches      780      824       +44     
===========================================
+ Hits         1028     1696      +668     
+ Misses       2346     1582      -764     
- Partials       74      177      +103     
Flag Coverage Δ
unittests 48.85% <43.75%> (+19.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sonichi sonichi added this pull request to the merge queue Nov 20, 2023
Merged via the queue into main with commit ef1c3d3 Nov 20, 2023
58 checks passed
@afourney afourney deleted the cache_703 branch November 20, 2023 21:11
whiskyboy pushed a commit to whiskyboy/autogen that referenced this pull request Apr 17, 2024
* Avoid creating cache database when cache_seed is None (which disables cache)

* Add some debugging.

* Removed some debugging.

* Update autogen/oai/client.py

Co-authored-by: Chi Wang <[email protected]>

* Fixed missing filter function logic from oai/client.py

---------

Co-authored-by: Chi Wang <[email protected]>
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.

cache_seed None doesn't prevent cache file creation in 0.2.0b5
4 participants