Skip to content

Add loguru for logging #282

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

Merged
merged 4 commits into from
May 31, 2023
Merged

Add loguru for logging #282

merged 4 commits into from
May 31, 2023

Conversation

raghu017
Copy link
Contributor

  • Add loguru for logging
  • Change PII logging to debug only

@vicondoa
Copy link

vicondoa commented May 25, 2023

Hi @isafulf, this is part of the improvements we are making for Azure Open AI. This project already had a dependency on loguru (by indirect reference), so @raghu017 is migrating all of the print statements to it along with switching all PII to debug statements. Let us know if there is anything you need from us.

@raghu017
Copy link
Contributor Author

@isafulf Can you take a look at the PR?

Copy link
Collaborator

@isafulf isafulf 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 doing this! Have you tested this yourself?

@@ -252,7 +253,7 @@ def create_results(data):
QueryResult(query=query.query, results=results)
)
except Exception as e:
print("error:", e)
logger.exception("error:", e)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't logger.exception already logs the stack trace along with the error message?
So: logger.exception("<descriptive message>") should be sufficient, this applies to all cases in the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed new changes, replaced exception with error. Exception is verbose stack trace and it is logging PII. Yes this is tested.

Copy link
Collaborator

@isafulf isafulf 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!

@isafulf isafulf merged commit 36354a2 into openai:main May 31, 2023
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