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

perf: init loggers once #5383

Closed
wants to merge 5 commits into from
Closed

Conversation

beckermr
Copy link
Contributor

@beckermr beckermr commented Jun 21, 2024

Description

This PR initializes the loggers once at the top of each module to hopefully reduce overheads.

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

closes #5382

@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Jun 21, 2024
@beckermr beckermr marked this pull request as ready for review June 21, 2024 14:58
@beckermr beckermr requested a review from a team as a code owner June 21, 2024 14:58
@beckermr
Copy link
Contributor Author

beckermr commented Jun 21, 2024

@conda/builds-tools This PR is ready for review!

Note that the python 3.12 failures are due to a deprecation warning for python 3.14 that comes out of code in conda-package-handling.

@dholth
Copy link
Contributor

dholth commented Jun 21, 2024

How much do we understand about the original behavior, besides that conad-build does strange things to logging (store all messages and suppress duplicates?) as a side effect of importing its logging utility? There was a comment about the user being able to override settings?

@beckermr
Copy link
Contributor Author

I understand nothing and yes user overrides are really hard.

FWIW, the current code base has a mix of making the loggers once at the top of each module versus making them on-the-fly. This PR simply changes all of the loggers to be made at the top of each module. So apparently the loggers do work when imported at the top. I'd like to defer cleaning up and/or changing the logging usage to another PR.

Copy link

codspeed-hq bot commented Jun 21, 2024

CodSpeed Performance Report

Merging #5383 will not alter performance

Comparing beckermr:init-logger-once (5674c02) with main (ee7d0ce)

Summary

✅ 3 untouched benchmarks

@beckermr beckermr mentioned this pull request Jun 21, 2024
3 tasks
@beckermr beckermr marked this pull request as draft June 21, 2024 17:29
@beckermr
Copy link
Contributor Author

I am closing this in favor of a smaller fix in #5384.

@beckermr beckermr closed this Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed [bot] added once the contributor has signed the CLA
Projects
Status: 🏁 Done
Development

Successfully merging this pull request may close these issues.

utils.get_logger dominates rerender performance
3 participants