Skip to content

feat: added set_logging_level functionality#372

Closed
pranayyb wants to merge 7 commits intoPrunaAI:mainfrom
pranayyb:feat/logging-level
Closed

feat: added set_logging_level functionality#372
pranayyb wants to merge 7 commits intoPrunaAI:mainfrom
pranayyb:feat/logging-level

Conversation

@pranayyb
Copy link
Copy Markdown
Contributor

@pranayyb pranayyb commented Oct 1, 2025

Description

Added a utility function set_logging_level to configure the global pruna_logger logging level programmatically or via the PRUNA_LOG_LEVEL environment variable. Also added unit tests to verify correct behavior for valid and invalid logging levels.

Related Issue

Fixes #351

Type of Change

  • New feature

How Has This Been Tested?

  • Programmatically setting logging levels (DEBUG, INFO, WARNING, ERROR, CRITICAL)
  • Setting logging levels via environment variable (PRUNA_LOG_LEVEL)
  • Validation that invalid logging levels raise ValueError
  • All tests pass locally using pytest

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my feature works
  • New and existing unit tests pass locally with my changes

Additional Notes

  • Default logging level is INFO if none is provided and environment variable is not set.
  • Function raises a clear error message if an invalid logging level is passed.

Note

Introduce set_logging_level to configure logger level via argument or PRUNA_LOG_LEVEL and update logger setup; add tests for valid/invalid levels.

  • Logging:
    • Add set_logging_level(logger, level) with validation and _LOG_LEVELS map; reads PRUNA_LOG_LEVEL when level is None.
    • Update setup_pruna_logger() to use set_logging_level (defaulting via env) and adjust docstring accordingly.
  • Tests:
    • Add tests/config/test_logging.py covering programmatic level setting, env-driven setting, and invalid level error.

Written by Cursor Bugbot for commit 5acaf26. This will update automatically on new commits. Configure here.

Copy link
Copy Markdown
Member

@johannaSommer johannaSommer left a comment

Choose a reason for hiding this comment

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

Looks perfect to me, thanks a lot @pranayyb for taking the time to enable it! Just have a quick question about the environment variable, then we can already merge!

Comment thread src/pruna/logging/logger.py Outdated
}


def set_logging_level(level: str | None = None) -> None:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Say the user sets the environment variable before running the script - the way i understand the code currently they would still have to call set_logging_level, is that correct? Can we implement it so that the logger - when first set up - respects this environment variable? Do you think that makes sense?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes i see, i'll change accordingly

cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

@pranayyb
Copy link
Copy Markdown
Contributor Author

pranayyb commented Oct 1, 2025

@johannaSommer could you please review now?

@johannaSommer johannaSommer self-requested a review October 2, 2025 15:20
Copy link
Copy Markdown
Member

@johannaSommer johannaSommer left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the change, I think now it looks great w.r.t. respecting the environment variable at setup! Is there a reason the user now has to pass the logger? In pruna, we currently only have one so it would be easier if the user only gives the logging level as an argument to set_logging_level? Otherwise LGTM, I will already run the test workflows so that we can merge after adressing that 😊

@pranayyb
Copy link
Copy Markdown
Contributor Author

pranayyb commented Oct 2, 2025

I did think of that without having to pass the logger but using set_logging_level required purna logger already defined and to define purna logger we need to call setup_peuna_logger which needed set_logging_level as you said at setup.
So it created a circular dependency.

#372 (comment)

Here the cursor bot raised that issue so I changed it that way.

Please let me know if any changes required.

@pranayyb pranayyb requested a review from johannaSommer October 4, 2025 11:35
@johannaSommer
Copy link
Copy Markdown
Member

Thanks a lot for the explanation @pranayyb ! In that case, would it make sense to call the set_logging_level function outside of the setup_pruna_logger function right after the setup call here?

@pranayyb
Copy link
Copy Markdown
Contributor Author

pranayyb commented Oct 7, 2025

hey @johannaSommer this was getting messy so raised a new clean PR with the final changes.
Here's the reference to the PR #398.

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.

[FEATURE] Add functionality to adjust the logging level for pruna_logger

2 participants