Add possibility to select logging level. #2786
Conversation
promptflow-core test result0 tests 0 ✅ 0s ⏱️ Results for commit 06f8926. ♻️ This comment has been updated with latest results. |
promptflow SDK CLI Azure E2E Test Result nirovins/change_logging_mechanism 4 files 4 suites 5m 1s ⏱️ Results for commit 06f8926. ♻️ This comment has been updated with latest results. |
SDK CLI Global Config Test Result nirovins/change_logging_mechanism4 tests 4 ✅ 1m 13s ⏱️ Results for commit 06f8926. ♻️ This comment has been updated with latest results. |
promptflow-evals test result12 files 12 suites 2m 13s ⏱️ Results for commit 06f8926. ♻️ This comment has been updated with latest results. |
Executor Unit Test Result nirovins/change_logging_mechanism779 tests 779 ✅ 3m 42s ⏱️ Results for commit 06f8926. ♻️ This comment has been updated with latest results. |
Executor E2E Test Result nirovins/change_logging_mechanism238 tests 232 ✅ 5m 9s ⏱️ For more details on these failures, see this check. Results for commit 06f8926. ♻️ This comment has been updated with latest results. |
SDK CLI Test Result nirovins/change_logging_mechanism 4 files 4 suites 1h 5m 22s ⏱️ For more details on these failures, see this check. Results for commit 06f8926. ♻️ This comment has been updated with latest results. |
…irovins/change_logging_mechanism
| @@ -145,6 +146,7 @@ def __init__( | |||
| **kwargs, | |||
| ): | |||
| self.variant = kwargs.pop("variant", None) or {} | |||
| self._log_level = kwargs.pop("log_level", None) | |||
There was a problem hiding this comment.
I think this setting should not be in Flow obj as it's changes static loggers which is not part of flow
| @@ -30,7 +33,7 @@ def __init__(self, project_scope: dict, credential: Optional[TokenCredential] = | |||
| # Load the flow as function | |||
| current_dir = Path(__file__).resolve().parent | |||
| flow_dir = current_dir / "flow" | |||
| self._flow = load_flow(source=flow_dir) | |||
| self._flow = load_flow(source=flow_dir, log_level=log_level) | |||
There was a problem hiding this comment.
could we directly set the logger here?
There was a problem hiding this comment.
Or just give user some doc that they can change logging behavior by setting some env variable.
There was a problem hiding this comment.
The main problem here is that all the log records, the task are not coming from flow itself, it is from invoker/executor infrastructure.
We are using two mechanisms of getting loggers. If user will set the PF_LOGGING_LEVEL, the flow executor logs will ignore it, because the environment variable will have to be set before importing promptflow, as the logger is initialized during import (see the logger_utils.py).
flow_logger = get_logger("execution.flow")Another problem is that we initialize the "flowinvoker" logger on flow invoker level, using different mechanism, which will respect PF_LOGGING_LEVEL (the code is given before this PR):
self.logger = kwargs.get("logger", LoggerFactory.get_logger("flowinvoker"))So that the logger created/set on the flow level will not have the effect. All
|
Hi, thank you for your interest in helping to improve the prompt flow experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. |
github-actions
bot
added
the
no-recent-activity
|
Hi, thank you for your contribution. Since there has not been recent engagement, we are going to close this out. Feel free to reopen if you'd like to continue working on these changes. Please be sure to remove the |
Description
This PR address the issue from the work item 3096911. Essentially, we want to control the logging verbosity. I have made the changes in the logging and added unit test.
To better test changes I have also added mypy configuration to pyproject.toml and fixed all the encountered issues.
All Promptflow Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines