Script to post comments on Feedback PRs

ssardina commented 3 months ago

As discussed with @AndrewPaulChester just now, we want a script that goes through each repo in the repo.csv database and makes a comment in the repo Feedback PR to paste the marking results.

The input will be:

CSV file of the MARKING spreadsheet
Text file of the autograder marking

This way we get rid of bulk email via YAMM and links to Google Drive for the report, everythign stays within the feedback PR in the Github repos.

We will use the PyGithub Python library, here is an example how to add a a comment:

https://pygithub.readthedocs.io/en/latest/examples/PullRequest.html?highlight=pull%20request

Basically, we want to paste a form of this table which used to be sent by email to students:

let's do it!! :muscle:

AndrewPaulChester commented 3 months ago

@ssardina we want to use the issue comment feature, not the pr-review-comment feature. You were right when you said that the review comments were the special ones attached to specific lines of code. Instead, since all PRs are issues, we just want to make a standard issue comment.

AndrewPaulChester commented 3 months ago

See https://github.com/RMIT-COSC1127-1125-AI24/p0-warmup-ssardina/pull/1 for a basic proof-of-concept I have working.

AndrewPaulChester commented 3 months ago

We discussed a few different ways to attach the report itself:

Attach as a file in the PR comment
Post as a second comment to the PR
Commit into the repo and link from the PR comment.

It does not appear that option 1 is possible through the open API. Option 3 I don't really like - it feels a bit strange to commit feedback as a file directly into their repos.

I would vote for option 2 - The nice thing is that even though the report can be long, it is already in markdown, so posting it as a comment is actually nicer to read than as a straight file like they used to get in the folder.

AndrewPaulChester commented 3 months ago

I've added a script that should basically work in 3e653bd. A few things to note:

My biggest concern is what if we have errors and have to selectively retry some students only. There is the capability to do this by digging through the logs and handling it manually, but if this isn't piped into a file then the info could be lost. I wonder if it would be better to have an explicit list of either successes or failures which could interact with the --repos argument.
Further formatting can be done to the table as required

ssardina commented 3 months ago

OK done with the post, now going into this full mode...

ssardina commented 3 months ago

My biggest concern is what if we have errors and have to selectively retry some students only. There is the capability to do this by digging through the logs and handling it manually, but if this isn't piped into a file then the info could be lost. I wonder if it would be better to have an explicit list of either successes or failures which could interact with the --repos argument.

mm I am not following. You mean we need to "remark" a student?

We can add an option to the script to restrict the posting to a particular GH username (repo), I have that with almost all scripts. We can also add an option to include a top message, like "REMARKING",

We can also do it manually as well, pasting ourselves, etc.. but the script can do it as well for 1 student right? or a list of students.

or did I get it wrong what you said?

AndrewPaulChester commented 3 months ago

Not quite, I'm not talking about remarking, but if the script itself fails for some students so they don't get the initial comments.

I'm imaging a scenario where we run into some rate limiting issue with the API so 30% of students have randomly errored in no particular pattern. We can copy paste those students from the logs, but would be hugely time consuming. If we store the errors just bare as a file that can then be piped into the --repos option like you mentioned.

I'm 75% done with a patch to help with this.

AndrewPaulChester commented 3 months ago

@ssardina I've just pushed an update that has better error handling. I will try to pretty up the table section of the report a bit now, but if there's something more important let me know

ssardina commented 3 months ago

OK great yes I agree with your strategy, we will send all the console report to file and we can also catch and report the error repos, I suspect that is what you did. I will now try it

great, yes pretty print would be good while I give it a try

AndrewPaulChester commented 3 months ago

@ssardina Done - see the latest comment for a preview https://github.com/RMIT-COSC1127-1125-AI24/p0-warmup-ssardina/pull/1

I think this is functional for p0. I can see many more improvements that could be done for p1 (e.g. refactoring the markdown for reuse between projects), but in the interest of time this seems good to me.

I'll be around for another hour or so, let me know if I can help in any other way.

ssardina commented 3 months ago

Tahnks @AndrewPaulChester ! checking it

ssardina commented 3 months ago

@AndrewPaulChester did you push the very late changes for pretty print?

AndrewPaulChester commented 3 months ago

While I'm thinking about it, other things we could consider improving in this process:

[x] Tagging the student using @username format so they automatically get a notification.
[ ] Updating the tables: some of the rows have fairly cryptic names, with the added markdown flexibility we might be able to do better.
[ ] Refactoring the markdown table to allow reuse of the shared components between projects, while the question specific format can be easily swapped out.
[ ] Consider making a basic comment for students who did not successfully submit, either due to lack of certification, or lack of the correct tag. This would require a slight code change - currently we simply ingest the entire CSV file and put it into the template string immediately. If we want to programatically access the values in the spreadsheet columns we would need to load the csv as a dict/list of dicts and then generate the text later.

ssardina commented 3 months ago

Done the first one!

https://github.com/RMIT-COSC1127-1125-AI24/p0-warmup-ssardina/pull/1

ssardina commented 3 months ago

OK fuly completed and done, I added a lot of improvements, the main being:

Get cases with no certification or no submission, with targeted messages
Get report file name from CSV file now (that needed extending grader system); bc some reports are xxx_ERROR.txt
Ability to give number limits on which repos to process, for example 10 to 20
repo name non-sensitive for case
more that I cannot remember!

I have already sent the first pack foir AI24 P0 with this:

python ../git-hw-submissions.git/gh_pr_feedback_comment.py repos.csv marking-p0.csv reports  -t ~/.ssh/keys/gh-token-ssardina.txt -s 0 -e 10 |& tee pr_feedback_0-10.log
Thu, 08 Aug 2024 02:09:41 INFO     Starting on Australia/Melbourne: 2024-08-08T12:09:41.654728+10:00

Thu, 08 Aug 2024 02:09:41 INFO     Namespace(REPO_CSV='repos.csv', MARKING_CSV='marking-p0.csv', REPORT_FOLDER='reports', repos=None, token_file='/home/ssardina/.ssh/keys/gh-token-ssardina.txt', start=0, end=10)
Thu, 08 Aug 2024 02:09:41 INFO     Processing repo 1/11: ssardina (https://github.com/RMIT-COSC1127-1125-AI24/p0-warmup-ssardina)...
Thu, 08 Aug 2024 02:09:41 ERROR          Repo RMIT-COSC1127-1125-AI24/p0-warmup-ssardina not found in marking-p0.csv.
Thu, 08 Aug 2024 02:09:41 INFO     Processing repo 2/11: msardina (https://github.com/RMIT-COSC1127-1125-AI24/p0-warmup-msardina)...
Thu, 08 Aug 2024 02:09:41 ERROR          Repo RMIT-COSC1127-1125-AI24/p0-warmup-msardina not found in marking-p0.csv.
Thu, 08 Aug 2024 02:09:41 INFO     Processing repo 3/11: k-z-z (https://github.com/RMIT-COSC1127-1125-AI24/p0-warmup-k-z-z)...
Thu, 08 Aug 2024 02:09:45 INFO     Processing repo 4/11: s3897720 (https://github.com/RMIT-COSC1127-1125-AI24/p0-warmup-s3897720)...
Thu, 08 Aug 2024 02:09:49 INFO     Processing repo 5/11: r-jinkies (https://github.com/RMIT-COSC1127-1125-AI24/p0-warmup-R-Jinkies)...
Thu, 08 Aug 2024 02:09:53 INFO     Processing repo 6/11: zahoorleghari1 (https://github.com/RMIT-COSC1127-1125-AI24/p0-warmup-ZahoorLeghari1)...
Thu, 08 Aug 2024 02:09:57 INFO     Processing repo 7/11: salmaan1234 (https://github.com/RMIT-COSC1127-1125-AI24/p0-warmup-salmaan1234)...
Thu, 08 Aug 2024 02:10:01 INFO     Processing repo 8/11: lukegasbarro (https://github.com/RMIT-COSC1127-1125-AI24/p0-warmup-lukegasbarro)...
Thu, 08 Aug 2024 02:10:05 INFO     Processing repo 9/11: s3976304 (https://github.com/RMIT-COSC1127-1125-AI24/p0-warmup-s3976304)...
Thu, 08 Aug 2024 02:10:09 INFO     Processing repo 10/11: bryanpham132 (https://github.com/RMIT-COSC1127-1125-AI24/p0-warmup-bryanpham132)...
Thu, 08 Aug 2024 02:10:14 INFO     Processing repo 11/11: asoftrac5 (https://github.com/RMIT-COSC1127-1125-AI24/p0-warmup-asoftrac5)...
Thu, 08 Aug 2024 02:10:18 INFO     Finished! Total repos: 11 - Errors: 2.
Thu, 08 Aug 2024 02:10:18 INFO     Repos with errors written to errors_pr.csv.

See the 2 errors were repo ssardina and msardina bc they do not have marking! All good.

ssardina-teaching / git-hw-submissions

Script to post comments on Feedback PRs #20