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

[BUG] Map task logging level fixed at 30 for default logger #3032

Merged
merged 2 commits into from
Jan 6, 2025

Conversation

pvditt
Copy link
Contributor

@pvditt pvditt commented Jan 4, 2025

Tracking issue

closes flyteorg/flyte#5961
fixes: https://linear.app/unionai/issue/DX-1155/investigate-logging-level-behavior-in-map-tasks

Why are the changes needed?

Logging for map tasks always defaults to default since logging library is re-imported in array node map task after being initialized in the wf code's files

What changes were proposed in this pull request?

remove logging import and utilize flytekit logger instead

How was this patch tested?

built an image w/ the changes and ran:

import logging

from flytekit import task, workflow, map_task

level = logging.INFO
logging.basicConfig(level=level)

@task
def my_task(s: str) -> str:
    logger = logging.getLogger()
    print(f"Effective logging level: {logger.getEffectiveLevel()}")

    # Log something
    logging.info("This is an info log message")
    logging.error("This is an error log message")
    return s


@workflow
def wf():
    my_task(s='hello world')
    strings = ['hello', 'world']
    return map_task(my_task)(s=strings)

Setup process

(venv) ➜  flytesnacks git:(master) ✗ k logs -n flytesnacks-development   arvs4wm4hb2j28mxk4w8-n0-0 

INFO:root:This is an info log message
ERROR:root:This is an error log message
Effective logging level: 20
(venv) ➜  flytesnacks git:(master) ✗ k logs -n flytesnacks-development   arvs4wm4hb2j28mxk4w8-n1-0-n1-0
INFO:root:This is an info log message
ERROR:root:This is an error log message
Effective logging level: 20

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

Summary by Bito

Fixed a logging level inconsistency in map tasks by removing direct logging import in array_node_map_task.py and implementing flytekit logger instead. This ensures logging levels are properly inherited between workflow code and map tasks, resolving the issue where map tasks were fixed at level 30.

Unit tests added: False

Estimated effort to review (1-5, lower is better): 1

@flyte-bot
Copy link
Contributor

Code Review Agent Run Status

  • Limitations and other issues: ❌ Failure - The AI Code Review Agent skipped reviewing this change because it is configured to exclude certain pull requests based on the source/target branch or the pull request status. You can change the settings here, or contact the agent instance creator at [email protected].

@flyte-bot
Copy link
Contributor

Code Review Agent Run Status

  • Limitations and other issues: ❌ Failure - The AI Code Review Agent skipped reviewing this change because it is configured to exclude certain pull requests based on the source/target branch or the pull request status. You can change the settings here, or contact the agent instance creator at [email protected].

@pvditt pvditt marked this pull request as ready for review January 6, 2025 21:00
@pvditt pvditt changed the title don't import logging in array node map task [BUG] Map task logging level fixed at 30 for default logger Jan 6, 2025
@pingsutw pingsutw merged commit 9c2caa1 into master Jan 6, 2025
102 checks passed
@flyte-bot
Copy link
Contributor

flyte-bot commented Jan 6, 2025

Code Review Agent Run #75e525

Actionable Suggestions - 0
Review Details
  • Files reviewed - 1 · Commit Range: f0a7f3f..c6ad09a
    • flytekit/core/array_node_map_task.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by Bito Logo

@flyte-bot
Copy link
Contributor

Changelist by Bito

This pull request implements the following key changes.

Key Change Files Impacted
Bug Fix - Map Task Logger Level Fix

array_node_map_task.py - Replace direct logging import with flytekit logger to fix logging level inconsistency

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.

[BUG] Map task logging level fixed at 30 for default logger.
3 participants