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

Enable pytest live log and show warning logs on GitHub Actions CI runs #35912

Merged
merged 5 commits into from
Feb 10, 2025

Conversation

ydshieh
Copy link
Collaborator

@ydshieh ydshieh commented Jan 27, 2025

We want to have warnings being logged on GitHub Actions run.

(currently on main, only those given by warnings.warn(...) are saved when using --make-reports)

@ydshieh
Copy link
Collaborator Author

ydshieh commented Jan 27, 2025

run-slow: convnextv2

1 similar comment
@ydshieh
Copy link
Collaborator Author

ydshieh commented Jan 27, 2025

run-slow: convnextv2

Copy link

This comment contains run-slow, running the specified jobs: ['models/convnextv2'] ...

@ydshieh
Copy link
Collaborator Author

ydshieh commented Jan 28, 2025

run-slow: convnextv2

Copy link

This comment contains run-slow, running the specified jobs: ['models/convnextv2'] ...

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@ydshieh
Copy link
Collaborator Author

ydshieh commented Jan 28, 2025

See a run with the live log (for warnings)

https://github.com/huggingface/transformers/actions/runs/13008702695/job/36281380509

@@ -101,7 +101,8 @@ def _configure_library_root_logger() -> None:
formatter = logging.Formatter("[%(levelname)s|%(pathname)s:%(lineno)s] %(asctime)s >> %(message)s")
_default_handler.setFormatter(formatter)

library_root_logger.propagate = False
is_ci = os.getenv("CI") is not None and os.getenv("CI").upper() in {"1", "ON", "YES", "TRUE"}
library_root_logger.propagate = True if is_ci else False
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need propagate = True in order to get live log (here for warnings), but let's only do it when CI env. variable is set.
This variable is set on GitHub Actions and CircleCI.

On CircleCI, we use -n xxxx (xdist) which won't show live log, which is preferred (to keep it more quite).
On GitHub Actions, we don't use -n xxx and live log is shown, so we can grab logs with warning info.

@ydshieh ydshieh marked this pull request as ready for review January 28, 2025 14:58
@ydshieh ydshieh changed the title [don't merge] check log Enable pytest live log and show warning logs on GitHub Actions CI runs Jan 28, 2025
@@ -785,8 +785,7 @@ def validate(self, is_init=False):
for arg_name in ("cache_implementation", "cache_config", "return_legacy_cache"):
if getattr(self, arg_name) is not None:
logger.warning_once(
no_cache_warning.format(cache_arg=arg_name, cache_arg_value=getattr(self, arg_name)),
UserWarning,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Having UserWarning (or anything else as 2nd arg.) is not going to work. It gives

    def getMessage(self):
        """
        Return the message for this LogRecord.

        Return the message for this LogRecord after merging any user-supplied
        arguments with the message.
        """
        msg = str(self.msg)
        if self.args:
>           msg = msg % self.args
E           TypeError: not all arguments converted during string formatting

Previously, some log handler can handle the above exception:

  • <StreamHandler <tempfile._TemporaryFileWrapper object at 0x000002AC2614E850> (NOTSET)>
  • <_LiveLoggingStreamHandler (WARNING)>,
  • <_FileHandler \.\nul (NOTSET)>

But after this PR, a new extra handle is used and the exception is kept and thrown

  • <LogCaptureHandler (NOTSET)>,

@ydshieh
Copy link
Collaborator Author

ydshieh commented Jan 28, 2025

run-slow: convnextv2

Copy link

This comment contains run-slow, running the specified jobs: ['models/convnextv2'] ...

@ydshieh ydshieh requested a review from ArthurZucker January 28, 2025 17:19
Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

Completely missed the ping sorry!

@ydshieh
Copy link
Collaborator Author

ydshieh commented Feb 10, 2025

run-slow: convnextv2

Copy link

This comment contains run-slow, running the specified jobs: This comment contains run-slow, running the specified jobs:

models: ['models/convnextv2']
quantizations: [] ...

Comment on lines +108 to +109
root = _get_library_root_logger()
with patch.object(root, "propagate", False):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It turns out that live log will interfere with CaptureStdout and cause this test failed.

Simply patch the root logger's propagate within with here.

@ydshieh
Copy link
Collaborator Author

ydshieh commented Feb 10, 2025

run-slow: convnextv2

Copy link

This comment contains run-slow, running the specified jobs: This comment contains run-slow, running the specified jobs:

models: ['models/convnextv2']
quantizations: [] ...

@ydshieh ydshieh merged commit 3897f2c into main Feb 10, 2025
26 of 27 checks passed
@ydshieh ydshieh deleted the check_gh_log branch February 10, 2025 12:36
sbucaille pushed a commit to sbucaille/transformers that referenced this pull request Feb 16, 2025
huggingface#35912)

* fix

* remove

* fix

---------

Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
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.

3 participants