MNT Add bot comment if Linter logs were not created by AnneBeyer · Pull Request #34726 · scikit-learn/scikit-learn

AnneBeyer · 2026-08-11T11:26:51Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Adds a check in case log files don't exist (i.e., if the linting step failed before the logs were created), such that a helpful bot comment is triggered in any case.

Since the bot comment is a workflow_run action, it should get triggered by pushing a commit in any open PR without having to merge main explicitly, once this PR is merged. Let's hope and see 🤞

First time contributor introduction

AI usage disclosure

I used AI assistance for:

Code generation (e.g., when writing an implementation or fixing a bug)
Research and understanding

Any other comments?

AnneBeyer · 2026-08-11T11:32:06Z

        run: mkdir -p "$ARTIFACTS_DIR"

      - name: Download artifact
+        continue-on-error: true


This is a bit more defensive than what happened in this case. Since the error occurred only with pyrefly, the first part of versions.txt was actually written by lint.yml and uploaded, so the download didn't actually fail here. This is only relevant if nothing was uploaded by the linting step (which will only generate a warning but no failure in the linting action).
I have no strong opinion if this will be needed, but I guess it doesn't hurt?

I would not expect the artifact download to fail in 99%+ cases, so let's keep this simple and remove this, until we see a specific use case where this would have been needed.

If you start down this defensive road, you could also say that any of the steps before get_comment.py can fail e.g. because of a glitch in the network and trying to handle all these edge cases is not worth it.

AnneBeyer · 2026-08-11T11:34:49Z

    versions : dict
        A dictionary with the versions of the packages.
    """
+    if not os.path.exists(versions_file):


Same as above, this file actually exists in the current case, this is just an additional precaution to ensure the script doesn't fail before the comment is posted. If you think this is too defensive, I can also remove this part.

This one I would keep after having looked at it a bit more, because if the pyrefly line to get the version was the first one (pytest is the first one right now), the versions file would have been empty I think.

Please add a comment explaining that we want the comment script to be tolerant of something going terribly wrong in the lint script and we want to always post a comment with the generic work-around of merging upstream/main

Also I don't think it matters that much in this particular case, but I would use "easier to ask for forgiveness than permission" pattern, see EAFP Python glossary:

try: with open(...) as f: # file exists code except FileNotFoundError: # file does not exist code

If the version file exists but is empty, an empty dict will be created here without the changes anyway. So I will remove this part here as well.

This change would only be needed to catch the case that anything goes wrong already in the pip command before the import (for whatever reason). Then, possibly no version file (and consequently no log file either) will be created, which will already cause a failure of the download action (thus we would also need the change in bot-lint-comment.yml).
(The upload in the linting step will not fail, as the default for if-no-files-found: there is warn: Output a warning but do not fail the action.)
But as you said above, that is maybe too much of an edge case to handle here.

lesteve

Thanks, a few comments and we can merge this I think.

In an ideal world, you would test this on your fork by recreating a similar situation as what is happening right now. In this case though, the changes are simple enough that I would try and crossed our fingers. In the worst case scenario we have another PR coming up.

lesteve · 2026-08-11T13:12:38Z

        run: mkdir -p "$ARTIFACTS_DIR"

      - name: Download artifact
+        continue-on-error: true


I would not expect the artifact download to fail in 99%+ cases, so let's keep this simple and remove this, until we see a specific use case where this would have been needed.

If you start down this defensive road, you could also say that any of the steps before get_comment.py can fail e.g. because of a glitch in the network and trying to handle all these edge cases is not worth it.

lesteve · 2026-08-11T15:11:17Z

    versions : dict
        A dictionary with the versions of the packages.
    """
+    if not os.path.exists(versions_file):


This one I would keep after having looked at it a bit more, because if the pyrefly line to get the version was the first one (pytest is the first one right now), the versions file would have been empty I think.

Please add a comment explaining that we want the comment script to be tolerant of something going terribly wrong in the lint script and we want to always post a comment with the generic work-around of merging upstream/main

Also I don't think it matters that much in this particular case, but I would use "easier to ask for forgiveness than permission" pattern, see EAFP Python glossary:

try: with open(...) as f: # file exists code except FileNotFoundError: # file does not exist code

lesteve · 2026-08-11T15:11:38Z

            "`upstream/main` ([link]("
            "https://scikit-learn.org/dev/developers/contributing.html"
-            "#how-to-contribute)) and push the changes. If you already have done "
+            "#development-workflow)) and push the changes. If you already have done "


Thanks for fixing the link 🙏!

lesteve · 2026-08-11T15:12:46Z

 def get_message(log_file, repo_str, pr_number, sha, run_id, details, versions):
-    with open(log_file, "r") as f:
-        log = f.read()
+    if os.path.exists(log_file):


Same thing add a comment to say that we want to be tolerant to something going terribly wrong in the lint workflow.

Similar comment about using EAFP pattern.

Thanks for the EAFP pointer!

betatim

Let's go :D

lesteve · 2026-08-12T13:12:54Z

Thanks a lot, I pushed a small tweak and set auto-merge.

One of the tweak is because I think it's possible the versions file does not exist (I said empty in my earlier message but actually I meant non-existent).

lesteve · 2026-08-12T13:56:59Z

Seems to work on the first attempt, sometimes this happens 🎉. See bot comment in one of my PR where upstream/main has not been merged: #34671 (comment)

AnneBeyer · 2026-08-13T09:15:57Z

Thank you for merging it @lesteve, but like I tried to explain before, for your tweak for the missing version file to work, we also need the continue-on-error part in bot-lint-comment.yml. If none of the files exist (which can happen if the install step in lint.yml fails before even the version file is created), the download step in bot-lint-comment.yml will fail before your check is ever executed. See the demonstration in my fork below:

Here, I uncommented the upload step:
https://github.com/AnneBeyer/scikit-learn/actions/runs/31679986799

And just to be sure, I also checked by simulating that the file creation doesn't run, but the upload step is performed:
https://github.com/AnneBeyer/scikit-learn/actions/runs/31680444061
(Note that the linter throws a warning in this case:
https://github.com/AnneBeyer/scikit-learn/actions/runs/31680424038)

Only when I add continue-on-error: True in main, the try-except will catch the missing files, like here:
https://github.com/AnneBeyer/scikit-learn/actions/runs/31683205263
(Note that the unable-to-downlaod error is still thrown, but the comment is posted in the PR nonetheless)

It doesn't have an effect in the current case in scikit-learn, because part of the version file is created before the error. But in general I would prefer if we reach a conclusion in the discussion (I'd like to know why you decided to add the one without the other) instead of you pushing to my branch and merging before I get a say.

lesteve · 2026-08-14T12:38:56Z

Ah right, thanks for the repeated explanation, and sorry I missed this the first time around by going too quickly 😅 ...

As you said download-artifact does fail if no files were uploaded (i.e. early failure that causes the version file and the lint output file to not even exist), I guess I naively thought that download-artifact would download nothing and the workflow would keep going.

Feel free to open a PR with the continue-on-error: true with a comment explaining why we need it and I'll try to merge it quickly 🙏!

But in general I would prefer if we reach a conclusion in the discussion (I'd like to know why you decided to add the one without the other) instead of you pushing to my branch and merging before I get a say.

I'll keep this in mind, sorry about this. To give you my perspective (not saying that it is more valid than yours): I am personally in a mood where I try to merge early and iterate, especially for things that doesn't affect scikit-learn users, like CI, developer docs, etc ... it's easy enough to open a follow-up PR to tweak things if needed.

AnneBeyer added 3 commits August 11, 2026 12:47

add step to create empty files if logs were not created

479f715

remove empty file creation

c270ab8

add handling if log files don't exist

29981b9

AnneBeyer commented Aug 11, 2026

View reviewed changes

re-trigger CI

439f407

AnneBeyer added this to Labs Aug 11, 2026

github-project-automation Bot moved this to Todo in Labs Aug 11, 2026

AnneBeyer moved this from Todo to PR waiting for reviews in Labs Aug 11, 2026

lesteve mentioned this pull request Aug 11, 2026

CI Add bot comment when linting job fails early #34719

Closed

lesteve reviewed Aug 11, 2026

View reviewed changes

AnneBeyer added 2 commits August 12, 2026 11:10

remove defensive download continuation

debc427

convert to EAFP and add comment

e45a0aa

betatim approved these changes Aug 12, 2026

View reviewed changes

Small tweak

989e6c7

lesteve enabled auto-merge (squash) August 12, 2026 13:12

lesteve merged commit 7bf10a5 into scikit-learn:main Aug 12, 2026
36 checks passed

github-project-automation Bot moved this from PR waiting for reviews to Done in Labs Aug 12, 2026

AnneBeyer deleted the add_linting_error_bot_message branch August 13, 2026 07:55

Uh oh!

Uh oh!

Conversation

AnneBeyer commented Aug 11, 2026

Reference Issues/PRs

What does this implement/fix? Explain your changes.

First time contributor introduction

AI usage disclosure

Any other comments?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lesteve left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

betatim left a comment

Choose a reason for hiding this comment

Uh oh!

lesteve commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

lesteve commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AnneBeyer commented Aug 13, 2026

Uh oh!

lesteve commented Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lesteve left a comment •

edited

Loading

lesteve commented Aug 12, 2026 •

edited

Loading

lesteve commented Aug 12, 2026 •

edited

Loading

lesteve commented Aug 14, 2026 •

edited

Loading