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

CodeRabbit Feedback #50985

Open
aliciaaevans opened this issue Sep 26, 2024 · 14 comments
Open

CodeRabbit Feedback #50985

aliciaaevans opened this issue Sep 26, 2024 · 14 comments

Comments

@aliciaaevans
Copy link
Contributor

aliciaaevans commented Sep 26, 2024

You may have noticed that the CodeRabbit bot has been enabled. Hopefully this will help catch errors that might otherwise be missed until after a PR is merged to master.

If you have any feedback (including positive feedback), please use this issue so we can keep the discussion in a central location and not only in individual PRs.

It would also be good to know if it's suggesting anything that conflicts with Bioconda documentation.

@aliciaaevans
Copy link
Contributor Author

There is currently issue with the CodeRabbit invalidly triggering the artifact fetching comment job. I've opened a PR to correct that. Thanks for your patience with the extra comments about missing artifacts (which can be ignored, or better yet, properly trigger the BiocondaBot to make sure artifacts were built). #50984

@martin-g
Copy link
Contributor

Issue: Rate Limit

E.g. #50445 (comment)
IMO it would be better if the bot does not write a comment at all instead of complaining about too many commits.

@martin-g
Copy link
Contributor

Issue: Too much content

E.g. #50445 (review)
On the web Github UI this content is collapsed but it is a huge text to read in the email notifications and the mobile app of Github.
I am not saying that CodeRabbit is not useful but due to its comments it is much harder now to see the "meaningful" comments.

@martin-g
Copy link
Contributor

Issue: Wrong suggestion noarch: false

#50961 (comment)
The bot suggests using noarch: false instead of suppressing should_be_noarch_generic lint but it does not work.

@martin-g
Copy link
Contributor

Issue: Useless suggestion || exit 1

E.g. https://github.com/bioconda/bioconda-recipes/pull/50973/files#r1777318835
The bot suggests to add || exit 1 to almost every command in build.sh.
IMO it would be better to either suppress this suggestion or suggest using set -e instead.

@martin-g
Copy link
Contributor

Positive feedback: The summary

E.g. #51009
I like the added summary section in the PR description !

@martin-g
Copy link
Contributor

Issue: sha256sum is not available

E.g. https://github.com/bioconda/bioconda-recipes/pull/51008/files#r1778397655
sha256sum (and maybe md5sum) are not available

@aliciaaevans
Copy link
Contributor Author

@martin-g Thanks for pointing out these things. I haven't tried it myself yet, but @johanneskoester mentioned that replying to the bot if it makes incorrect suggestions will help change the behavior, so feel free to also try that if there's a bad suggestion.

I don't have time at the moment to dig into configuration options, but there might be some ways we can reduce the clutter.

@marcelm
Copy link
Contributor

marcelm commented Oct 3, 2024

@aliciaevans Can I suggest to post about the bot also in the announcements issue #33333?

Regarding feedback, I encountered the bot the first time in #51125 just now and at least in that particular autobump PR, I found it to was too noisy. It did not add anything useful.

@rhpvorderman
Copy link
Contributor

I just saw it when reviewing the dnaio pr #51157 (comment)

The "summary" is bigger than the git diff and as such has noting to offer. Given the enormous energy usage of the AI, I think it is better to be disabled. That is not to say it is a bad tool per se, but in the context of updating recipe.yml files I do not see a value add. So my feeling is that it is a bad fit for bioconda.

@johanneskoester
Copy link
Contributor

I hear your concerns and I tend to agree. Let us stop this for now, and then review the PRs it has evaluated so far. We can then see what we can already adjust in the settings or whether there are changes we should approach the coderabbit people about. One thing is certainly that it can be completely disabled for autobump PRs. For others, I see a value even for the yaml files, but we should have a close look at all suggestions and ensure that none of them was misleading.

@johanneskoester
Copy link
Contributor

Just checked the settings (while searching for the disable button). Apparently more options have been added since I last checked. I have adjusted it now to not post all this clutter messages. Maybe that helps already. Let's give it another few days. Then, we can revisit and see if it shall still be disabled entirely.

@rhpvorderman
Copy link
Contributor

rhpvorderman commented Oct 7, 2024

This PR: #51188

It updates only a dependency (bugfix). As such only two lines are updated. This is the coderabbit AI feedback. I will do a sentence by sentence breakdown.

The pull request involves modifications to the meta.yaml file for the cutadapt package.

Correct.

The package version has been updated to "4.9",

Incorrect. There have been no updates to the version.

and the build number has been incremented from 1 to 2.

Correct.

The source section now includes a specified URL for the package tarball along with a SHA256 checksum for integrity verification.

Incorrect. Source URL was not updated and neither was the checksum. Both were already present.

In the requirements section, the minimum version of the dnaio dependency has been revised from >=1.2.0 to >=1.2.2.

Correct.

The build section retains the existing installation script and the stipulation to skip builds for Python versions below 3.8. The test section remains unchanged, including an import test for cutadapt and a command to verify the version. Additionally, the about and extra sections have not been altered, continuing to provide metadata about the package.

Correct, but a very long winded way to say: no other changes were made.

conclusion

Given that it hallucinates that changes where none were made I rate this tool 1/10. Misleading and therefore unhelpful. EDIT: But it certainly does make less spelling mistakes than I do whilst giving feedback.

@rhpvorderman
Copy link
Contributor

#51393 (review)

afbeelding

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants