ucbds-infra / otter-grader

A Python and R autograding solution
https://otter-grader.readthedocs.io
BSD 3-Clause "New" or "Revised" License
123 stars 64 forks source link

Otter Grade: Added Progress Monitoring #836

Closed sean-morris closed 3 weeks ago

sean-morris commented 1 month ago

Closes #827

coveralls commented 1 month ago

Pull Request Test Coverage Report for Build 10876103619

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
otter/grade/init.py 15 16 93.75%
otter/utils.py 14 25 56.0%
otter/grade/containers.py 2 16 12.5%
<!-- Total: 32 58 55.17% -->
Totals Coverage Status
Change from base Build 10675675307: -0.3%
Covered Lines: 3740
Relevant Lines: 4576

💛 - Coveralls
sean-morris commented 1 month ago

@chrispyles I needed the grade/init.py::main function to be async so that I could call it directly from my application passing an instance of queue.Queue. This is instead of calling otter grade using the cli command and the python subprocess library -- which never felt right to me.

I added the --print-progress flag so that progress could printed to the console when using CLI. I am open to whatever here Chris. A first set of moments.

chrispyles commented 1 month ago

This is instead of calling otter grade using the cli command and the python subprocess library -- which never felt right to me.

I'm not sure if this is a good idea -- by calling otter as a package directly from your server, if otter crashes, it could bring down your entire server. I think it's better to run otter in a separate process, which subprocess does. Alternatively, if you want to call otter programmaticaly but still in a separate process, I think the multiprocessing module can be used to spawn a separate process that won't take down your server if it crashes (but I haven't used this much so I'd double check that).

sean-morris commented 4 weeks ago

@chrispyles I needed the grade/init.py::main function to be async so that I could call it directly from my application passing an instance of queue.Queue. This is instead of calling otter grade using the cli command and the python subprocess library -- which never felt right to me.

I added the --print-progress flag so that progress could printed to the console when using CLI. I am open to whatever here Chris. A first set of moments.

@chrispyles Scratch all that. I used your multiprocessing module idea and this greatly simplified just about everything! I also removed the --print-progress piece altogether.

chrispyles commented 4 weeks ago

This is a pretty big change. I think this should go into a major release (i.e. v6) so can you please rebase against the beta branch and PR to it?

Also please don't forget to move this to the beta branch @sean-morris

sean-morris commented 4 weeks ago

This is a pretty big change. I think this should go into a major release (i.e. v6) so can you please rebase against the beta branch and PR to it?

Also please don't forget to move this to the beta branch @sean-morris

In order to do this, we should rebase the master branch into the beta branch and then bring over my commit from feature branch? Lame here. It is a mix of commits that is slightly nerve-racking to merge. @chrispyles

chrispyles commented 3 weeks ago

All of the commits in the master branch are already merged into the beta branch. you should just be able to rebase your branch onto beta, or checkout to the beta branch, create a new branch off of it, and cherry-pick your commits into it.

sean-morris commented 3 weeks ago

This is a pretty big change. I think this should go into a major release (i.e. v6) so can you please rebase against the beta branch and PR to it?

Also please don't forget to move this to the beta branch @sean-morris

In order to do this, we should rebase the master branch into the beta branch and then bring over my commit from feature branch? Lame here. It is a mix of commits that is slightly nerve-racking to merge. @chrispyles

let me take another look. I am screwed up about this -- I keep seeing commits that should already be in master showing up in my feedback branch commits when I rebase it

sean-morris commented 3 weeks ago

You are running ahead of me! I just changed the merge branch to be beta not master. Hope that was OK.

chrispyles commented 3 weeks ago

You are running ahead of me! I just changed the merge branch to be beta not master. Hope that was OK.

no worries!

sean-morris commented 3 weeks ago

@chrispyles when I make these requested changes -- should I be rebasing/force pushing to keep it all as one commit? What is the best practice here?

chrispyles commented 3 weeks ago

In general force pushing is a bad idea. You don't need to keep everything in a single commit, it's fine to have multiple. And now that the branch has all of the beta commits in it you shouldn't need to rebase again.