Skip to content

fix: Use logging instead of print during materialization#5805

Open
Shizoqua wants to merge 7 commits intofeast-dev:masterfrom
Shizoqua:fix/materialization-use-logger
Open

fix: Use logging instead of print during materialization#5805
Shizoqua wants to merge 7 commits intofeast-dev:masterfrom
Shizoqua:fix/materialization-use-logger

Conversation

@Shizoqua
Copy link

@Shizoqua Shizoqua commented Jan 1, 2026

Summary

This PR removes direct print() usage from Feast’s materialization flow and replaces it with standard library logging. This avoids writing to stdout from library code while still surfacing materialization progress and warnings via configured log handlers.

Changes

  • Replace print() calls in FeatureStore.materialize, materialize_incremental, and ODFV materialization helper logic with logger.info/logger.warning.
  • Remove unused colorama import (Fore, Style) after eliminating colored stdout output.

Testing

  • python -m compileall sdk/python/feast/feature_store.py

Open with Devin

@Shizoqua Shizoqua requested a review from a team as a code owner January 1, 2026 13:16
@Shizoqua
Copy link
Author

Shizoqua commented Jan 1, 2026

@franciscojavierarceo Good day
What am I to do next?

@Shizoqua
Copy link
Author

Shizoqua commented Jan 3, 2026

@franciscojavierarceo

Good day, my PR was approved 2 days ago but I haven't had any response since then.

@ntkathole
Copy link
Member

ntkathole commented Jan 5, 2026

@Shizoqua There are unit tests failing for this change. Also, I think this changes the default cli console output for feast materialize or materialize-incremental commands. Since default cli logging configured as warning, there is no console output for these commands after this change.

@Shizoqua
Copy link
Author

Shizoqua commented Jan 5, 2026

Hi @ntkathole ! thanks for the catch.
I have updated it as requested.

@Shizoqua Shizoqua force-pushed the fix/materialization-use-logger branch from ee9e526 to 007c26c Compare January 5, 2026 10:45
@franciscojavierarceo
Copy link
Member

@ntkathole that's true it does change the default output. That's probably the right thing actually. We need to make sure we set

import logging
# This sets the root logger level to INFO
logging.basicConfig(level=logging.INFO)

@Shizoqua can you ensure this?

This is actually probably much better as we can add logging.debug more intentionally in the code. At the moment we dump a lot of info.

@Shizoqua
Copy link
Author

Shizoqua commented Jan 7, 2026

This is actually probably much better as we can add logging.debug more intentionally in the code. At the moment we dump a lot of info.

I will start working on it now

@Shizoqua
Copy link
Author

Done — Feast CLI already configures logging via logging.basicConfig(...), but the default --log-level in this branch was warning, which suppressed the new logger.info(...) materialization output. I changed the CLI default to info so the output remains visible by default, and users can still override it via --log-level debug|warning|....

@Shizoqua
Copy link
Author

Good day @franciscojavierarceo
I would appreciate it if my PR is looked at and reviewed.

@Shizoqua
Copy link
Author

Good day @franciscojavierarceo
I would appreciate it if my PR is looked at.

Copy link
Member

@franciscojavierarceo franciscojavierarceo left a comment

Choose a reason for hiding this comment

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

unit tests are failing, can you revise?

@Shizoqua Shizoqua force-pushed the fix/materialization-use-logger branch from 4c71c27 to c309c53 Compare January 18, 2026 20:19
@Shizoqua
Copy link
Author

unit tests are failing, can you revise?

corrected the requested reviews you gave

@Shizoqua
Copy link
Author

Hello,
Good day @franciscojavierarceo @ntkathole
Just a quick reminder of my PR getting reviewed.

@franciscojavierarceo
Copy link
Member

need to fix tests and linter

@Shizoqua
Copy link
Author

need to fix tests and linter

I'll work on that immediately

@Shizoqua Shizoqua force-pushed the fix/materialization-use-logger branch from 2fb3c5a to d8bed89 Compare January 20, 2026 17:41
@Shizoqua
Copy link
Author

need to fix tests and linter

Done, I have worked on the tests and linter

Copy link
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 4 additional flags.

Open in Devin Review

@franciscojavierarceo
Copy link
Member

@Shizoqua can you provide screenshots of the CLI output before and after? Thanks a ton!

@Shizoqua
Copy link
Author

@Shizoqua can you provide screenshots of the CLI output before and after? Thanks a ton!

Before Screenshot:

b4

After Screenshot:

aft

Here are the CLI outputs before & after.

Command:
feast --log-level info materialize-incremental 2025-01-01T00:00:00

Before (upstream/master): output uses print (no INFO:feast.feature_store banner)
After (this PR): output uses logging (shows INFO:feast.feature_store:Materializing...)

@ntkathole
Copy link
Member

@Shizoqua Looking good now, please resolve the conflicts and we will merge

@Shizoqua Shizoqua force-pushed the fix/materialization-use-logger branch from d8bed89 to e2062f2 Compare January 31, 2026 22:21
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 1 new potential issue.

🟡 1 issue in files not directly in the diff

🟡 Colorama ANSI escape codes embedded in logger.info() calls will produce garbled output in log files (sdk/python/feast/feature_store.py:1509-1511)

The PR's goal is to replace print() with logging to avoid writing to stdout from library code. However, the code still uses colorama's Style.BRIGHT + Fore.GREEN and Style.RESET_ALL ANSI escape codes inside logger.info() calls.

Click to expand

Problem

ANSI escape codes are only meaningful when output goes to a terminal that interprets them. When these logger.info() messages are written to:

  • Log files
  • Log aggregation systems (e.g., CloudWatch, Splunk)
  • Non-TTY environments (CI/CD pipelines, Docker containers)
  • Any handler that doesn't support ANSI codes

The output will contain raw escape sequences like \x1b[1m\x1b[32mfeature_view_name\x1b[0m instead of colored text.

Example of affected code at sdk/python/feast/feature_store.py:1509-1511:

logger.info(
    f"{Style.BRIGHT + Fore.GREEN}{feature_view.name}{Style.RESET_ALL}:"
)

Expected behavior

Log messages should be plain text without ANSI codes:

logger.info("%s:", feature_view.name)

Impact

Log files and log aggregation systems will contain unreadable escape sequences, making logs harder to read and parse.

Recommendation: Remove colorama formatting from all logger.info() calls. Use plain text logging, e.g., logger.info("%s:", feature_view.name). Also remove the unused colorama import at line 37.

View issue and 6 additional flags in Devin Review.

Open in Devin Review

@Shizoqua
Copy link
Author

@Shizoqua Looking good now, please resolve the conflicts and we will merge

I have corrected the conflicts.

Shizoqua and others added 7 commits February 6, 2026 09:35
Signed-off-by: Shizoqua <hr.lanreshittu@yahoo.com>
Signed-off-by: Lanre Shittu <136805224+Shizoqua@users.noreply.github.com>
Signed-off-by: Lanre Shittu <136805224+Shizoqua@users.noreply.github.com>
Signed-off-by: Shizoqua <hr.lanreshittu@yahoo.com>
Signed-off-by: Shizoqua <hr.lanreshittu@yahoo.com>
Signed-off-by: Shizoqua <hr.lanreshittu@yahoo.com>
Signed-off-by: Shizoqua <hr.lanreshittu@yahoo.com>
Signed-off-by: Shizoqua <hr.lanreshittu@yahoo.com>
@ntkathole ntkathole force-pushed the fix/materialization-use-logger branch from d979b11 to e5d6e6a Compare February 6, 2026 04:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants