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

AutoGen logging using Azure Cosmos DB #2329

Closed
wants to merge 151 commits into from
Closed

Conversation

wmwxwa
Copy link
Collaborator

@wmwxwa wmwxwa commented Apr 8, 2024

Why are these changes needed?

Adding the connector to use Cosmos DB for logging

Related issue number

N/A

Checks

Copy link
Collaborator

@ekzhu ekzhu left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! A couple of things to add:

  1. Add handling for the cosmosdb logger in the factory method in
    def get_logger(logger_type: str = "sqlite", config: Optional[Dict[str, Any]] = None) -> BaseLogger:
    if config is None:
    with an import statement to import the new module, inside a try-except block to catch import error when cosmos db package is not installed.
  2. In setup.py add a new extra_require entry for cosmosdb similar to redis:

    autogen/setup.py

    Lines 49 to 50 in 5a96dc2

    extras_require={
    "test": [
  3. Add a unit test for the logger. @tonybaloney do you want to comment on how to add a mock test for cosmos db usage?

@ekzhu ekzhu mentioned this pull request Apr 10, 2024
3 tasks
autogen/logger/cosmos_db_logger Outdated Show resolved Hide resolved
autogen/logger/cosmos_db_logger Outdated Show resolved Hide resolved
Copy link
Contributor

@cheng-tan cheng-tan left a comment

Choose a reason for hiding this comment

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

Thank you for the PR.
To fix the pre-commit failure: run pre-commit run --all-files,
to fix the type issues run mypy autogen/logger/cosmos_db_logger.py.

Can you also add some unit test for cosmos db logger like Eric suggested above? Existing logging unit test is under test/test_logging.py

autogen/logger/cosmos_db_logger.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Apr 12, 2024

Codecov Report

Attention: Patch coverage is 34.28571% with 69 lines in your changes are missing coverage. Please review.

Project coverage is 30.48%. Comparing base (372ac1e) to head (508261f).
Report is 18 commits behind head on main.

Files Patch % Lines
autogen/logger/cosmos_db_logger.py 37.34% 51 Missing and 1 partial ⚠️
autogen/logger/logger_factory.py 23.07% 10 Missing ⚠️
autogen/runtime_logging.py 0.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2329      +/-   ##
==========================================
- Coverage   33.11%   30.48%   -2.64%     
==========================================
  Files          86       87       +1     
  Lines        9108     9340     +232     
  Branches     1938     2137     +199     
==========================================
- Hits         3016     2847     -169     
- Misses       5837     6158     +321     
- Partials      255      335      +80     
Flag Coverage Δ
unittest 12.81% <34.28%> (?)
unittests 29.71% <34.28%> (-3.41%) ⬇️

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.

@wmwxwa
Copy link
Collaborator Author

wmwxwa commented May 22, 2024

Cannot revolve conflict. Continuing the work under PR 2760

@wmwxwa wmwxwa closed this May 22, 2024
@wmwxwa wmwxwa reopened this May 22, 2024
@Hk669
Copy link
Contributor

Hk669 commented Jun 3, 2024

@wmwxwa had come across an other PR #2760 . if that's the updated one. can this be closed?

@wmwxwa wmwxwa closed this Jun 3, 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.

8 participants