Allow tamanager to add access at an assignment level

[dmusican]

In the upper level courses that I teach, I regularly use one or two students in that course who also work as the course graders. (I'm at an undergrad only institution, and we often don't have students otherwise available to grade our advanced classes.) The workflow is that they submit their work to me --- which I grade --- and once they have submitted their work, I provide detailed grading rubrics and then grant them access to the other students' submissions.

It would be great if the tamanager plugin would allow me to grant them access to all repos associated with a particular assignment, instead of just with the course as a whole.

In reading the tamanager documentation, it looks like I can almost fake it by using tamanager non-persistently, and just running it after my graders have submitted their work. That comes close, but they will still inadvertently gain access to repos for future assignments that some students have started early on. (Or is there some other workaround that I've missed?)

In the past, I've handled this situation just by cloning the repos myself and distributing those repos to the graders, but it looks like RepoBee almost has a much better solution for this, which would be fantastic.

If this is a relatively easy fix to the plugin that you'd like me to contribute, I'd be grateful if you'd drop a tip or two on how to do it and I can give it a try. I'm wondering if adding a command line argument for the assignment, and then doing some sort of test for it in the plugin code, is all it takes.

[slarse]

Hmm, that is a super interesting use case I think should be supported. I'm relatively certain you could hack it with the currently existing functionality, like so:

1. Manually create the repobee-teachers team and add your grader students to it (with whatever access rights they need).
2. Run repobee -p tamanager repos setup --assignments <the assignments in question> [...]. This will add all of the repos for the specified assignments to the repobee-teachers team, and thus the graders you added to it will be granted access to them. No other repos are added.
3. When grading is done, delete the repobee-teachers team and all of the graders will have their access rights revoked. Rinse and repeat for the next assignment.

Of course, this only works if you don't allow grading of more than one assignment at a time.

As for making this more user friendly, there are two ways we could go about it. Either we adapt the tamanager plugin to support this use case, or we implement a plugin for the peer review allocation such that you can provide a set of students to be reviewed, and a separate set of students to act as reviewers. Have a look at the peer review docs and let me know what you think of those two options.

Also, is the reviewing single-blind (i.e. reviewed students don't know who the reviewers are)?

[slarse]

Once we've decided on the best way to implement this, I can give you an outline of how to do it and you can have a go at it. It would make for a great first contribution to the code base for you!

[dmusican]

Thanks for the response -- and that's cool that the built-in functionality will already do it. That will certainly get me rolling with the course I've got coming up.

Regarding making it more user-friendly and adding it to the system: I've spent a little time looking at both of your options and I keep going back and forth. From a user perspective, it seems that it more naturally belongs in the tamanager plugin because that's where I started, but I also see what might be trickiness there in that the teams add-teachers command is really intended to provide access to everything. The peer review functionality is already set up to work with a particular assignment, and this is in some sense a form of peer review. So, to the extent that I understand the architecture (which I admittedly don't really yet), it looks like it might be a more straightforward and manageable fix to be a plugin for peer review allocation.

I might be talking myself into the latter as I'm writing this. It would even then support things like one student grader gets one half of the class, and the other student grader gets the other half of the class, etc.

So yeah, something that looks like (this is just a concept)

repobee reviews assign -a task-1 --reviewers-file reviewers.txt --students-file students.txt

might do it?

And yes, it's typically single-blind. I would honestly love double-blind, but I have found that more cumbersome than it's worth when dealing with GitHub repos. If you've got ideas on that I'm all ears.

[slarse]

Regarding making it more user-friendly and adding it to the system: I've spent a little time looking at both of your options and I keep going back and forth. From a user perspective, it seems that it more naturally belongs in the tamanager plugin because that's where I started, but I also see what might be trickiness there in that the teams add-teachers command is really intended to provide access to everything. The peer review functionality is already set up to work with a particular assignment, and this is in some sense a form of peer review. So, to the extent that I understand the architecture (which I admittedly don't really yet), it looks like it might be a more straightforward and manageable fix to be a plugin for peer review allocation.

There are not-so-hard ways to make it work in the tamanager plugin as well, but in terms of semantics I think that the review functionality is a better fit. The one problem there, since it's single blind, is that a review issue is opened by default in the reviewed repo specifying the reviewer, so there would need to be a new argument or hook to disable opening that issue. Perhaps we could simply add the argument --single-blind to not specify the name of the reviewer in the review issue. I could add that, it would be very easy and makes sense to have.

I might be talking myself into the latter as I'm writing this. It would even then support things like one student grader gets one half of the class, and the other student grader gets the other half of the class, etc.

So yeah, something that looks like (this is just a concept)

repobee reviews assign -a task-1 --reviewers-file reviewers.txt --students-file students.txt

might do it?

Yup, something like that would do it from a CLI perspective. If the reviewers aren't that numerous, I'd probably specify them like so instead:

--reviewers alice bob eve

Implementing a plugin like that would be rather easy, see for example the pairwise plugin which does pairwise student allocations. You'd also need to make a command extension of the reviews assign command in order to add the --reviewers or --reviewers-file option. The thing that might prove hard for you is adding automated tests for the functionality. I think a good way for us to go about this would be for me to write or assist in writing the tests.

And yes, it's typically single-blind. I would honestly love double-blind, but I have found that more cumbersome than it's worth when dealing with GitHub repos. If you've got ideas on that I'm all ears.

See #791 for an idea for double-blind review. I think that's something that many teachers might want.

[slarse]

A problem is that reviews check won't work as expected with this new allocation scheme, as it expects all reviewing students to also have been reviewed themselves. However, as your reviews are single-blind, I'm assuming you don't intend for students to post reviews as issues on the GitHub issue tracker, and have some other way of collecting reviews. So, it's not an immediate problem for you, and fixing it can be postponed for later.

[dmusican]

This all sounds good, and many thanks. My winter term has just started this week and I've got fires burning everywhere, but this is important to me because I actually need this functionality. I hope to be in touch soon with an attempt at making this work!

[slarse]

Totally understand, I've got quite a bit of work to do to prepare RepoBee for SIGCSE myself, so I don't mind that we push this back a bit.

[slarse]

@dmusican I've got a few developments that you might be interested in.

First, we've got double-blind peer review functionality coming in RepoBee 3.6 (which will be releasing within two weeks). It's in alpha and the exact workings will not stabilize until RepoBee 4 (which will release this summer), but it works and I think it's pretty cool :). We tried it in action last week with around ~50 or so students, and it worked out really well. Docs are here explaining how it works: https://repobee.readthedocs.io/en/latest/peer.html#double-blind-peer-review

The second thing is that I'm planning a total redesign of how the peer review commands work, which will make the thing you requested here a lot easier to implement. This design change is necessary for the double-blind reviews to be more user-friendly (for the one using RepoBee, that is), but it will also nicely solve your problem. The plan is as follows:

reviews assign works as is, but also spits out a review allocation in a JSON file. All other commands take the review allocation file as an argument. Thus, a plugin can make an asymmetric assignment (which is what you need), and it will just work with the other commands as they simply read from the allocation file. You can even make it double-blind if you'd like!

[dmusican]

Fantastic; I'm glad to hear of both developments. I'm hoping to get to work on this during my spring break, which is in the middle of next month.

As an aside, RepoBee has been wonderful. I've been using it heavily in my upper-level course this winter, and it has been rock solid. Many thanks for making this available.

[slarse]

I'm hoping to get to work on this during my spring break, which is in the middle of next month.

That's great. Ping me when you're about to get started. If all goes according to plan, I should have implemented the new peer review commands. The functionality will essentially be the same, it'll just be packaged a more nicely from a user perspective. I plan to put them in alongside the current ones, which will be deprecated and finally removed when RepoBee 4 releases this summer. With those in place, a rudimentary implementation of your needs (i.e. assigning students in set X to review students in set Y) shouldn't be more than ~20 lines of code.

I'll of course help if you get stuck, but having people who aren't intimately familiar with RepoBee try to do stuff just based on the documentation is good for improving the docs :)

As an aside, RepoBee has been wonderful. I've been using it heavily in my upper-level course this winter, and it has been rock solid. Many thanks for making this available.

That's absolutely fantastic to hear. And you're very welcome!

[dmusican]

Ping me when you're about to get started.

Will do!

[dmusican]

I think I'm ready to get started -- that said, you may be swamped getting ready for SIGCSE, which is reasonable! (I'm attending but not presenting.) If you've got info to point me towards regarding the new functionality, let me know when you can.

[slarse]

@dmusican Great to hear you're about to get started.

I'm attending but not presenting.

Feel free to drop by the RepoBee demo if you've got the time :). It'll be during the demo slot on the 16th, 6pm-7.45pm CET. We'll be showing the bare basics along with a quick-tutorial for writing plugins, and then there'll be a 10-12 minute Q&A.

If you've got info to point me towards regarding the new functionality, let me know when you can.

It's not implemented yet as I've been quite busy, but I'm working on it as I type this and think I'll be done later today. I'm going to add in the new review commands that use an allocations file as a hidden preview feature that you can activate by setting an environment variable. See #867

[slarse]

@dmusican I've merged a first version of the new RepoBee 4 peer review commands into the master branch. If I've not made any massive mistakes, they should support asymmetric review assignments.

They're locked behind a hidden feature flag, see #868 for details.

[dmusican]

Fabulous, thanks. I'll try to attend the session; a brief tutorial on writing plugins seems like it will likely be helpful and worth waiting a few days for!

[dmusican]

@slarse I'm working on getting started; for the time being, I'm not worried about blinding at all, just to get going.

As you suggested, I'm going about it based on your advice by working on making a variation of the pairwise plugin, as a command extension. It makes sense. So here's my question...

I'm setting it up so that the user can specify the name of the student grader on the command line via a --grader option. (Maybe later I'll allow a file or a config option, we'll see, it's all pretty easy to shake that up.) In order to create a ReviewAllocation, I need to specify a pair of teams: the review team, and the reviewed team. Getting the reviewed team is easy: I get it from iterating over the teams supplied as a parameter to the generate_review_allocations hook. But getting the reviewer team is thornier.

• The PlatformAPI.create_team seems like one approach, but the generate_review_allocations hook doesn't have the API as a parameter, so I don't have easy access to it (or do I?).
• Alternatively, I could just require that the grader student/team must be one of the teams that was supplied in the list to the hook. That's manageable, but still clunky; I would need to iterate through the supplied teams and find the one that matches the one specified on the command line (and make sure I do that equality / membership test properly).

Am I going in the right direction here? Is there a best canonical way within the generate_review_allocations hook, or within a command extension that's wrapping around it, to create/find a team associated with a string specified on the CLI?

[slarse]

Hi Dave!

This is a not-so-evident part of the plugin API that's suffering from a bit of legacy and poor naming. Basically, there are two kinds of team representations: plug.StudentTeam, which is a local representation, and plug.Team, which is the platform API wrapper.

What you need to put into the review allocation is the local representation. Assuming your graders already have teams, you can create it like so:

import repobee_plug as plug

grader = "graders-username"
student_team = plug.StudentTeam(members=[grader])

If your graders don't have individual teams, you can create some for them first.

$ repobee teams create --students grader1 grader2 grader3

That should solve your problem!

I'm planning to rename some of these classes for RepoBee 4 to reduce confusion (letting the old names still work for a couple of minor releases, with a deprecation notice). It may be sufficient to keep e.g. plug.StudentTeam as-is, but rename the platform representations such as plug.Team to plug.PlatformTeam.

[dmusican]

Fantastic, thank you -- I will give this a try, it looks like exactly what I need.

[dmusican]

If I had waited one more day, I'd be a month later, but I'm early. :)

@slarse I've got a prototype of the approach that seems to be essentially working. I can prepare a proper pull request, but there's a bug I'd rather fix first, and I thought you might understand it just by looking at the code.

https://github.com/dmusican/repobee/blob/plugin-attempt/src/_repobee/ext/reviewothers.py

The problem is only with regards to output to the terminal. The very first student grader I add (jane) works great:

$ repobee -p gitea -p ~/currentwork/repobee/src/_repobee/ext/reviewothers.py \
    reviews assign -a task-2 --sf students.txt -u dmusicant \
    --bu https://gitea.repobee.org/api/v1 -o dave-template --to dave-template \
    -t  ...  --grader jane
[WARNING] The Gitea plugin is in very early development, use at your own risk!                           
Fetching teams and repos: 100%|████████████████████████████████████████████| 3/3 [00:04<00:00,  1.64s/it]
Allocating reviews
Assigning jane to review alice-task-2               
Assigning jane to review bob-task-2                 
Assigning jane to review ellen-task-2               
Creating review teams: 100%|███████████████████████████████████████████████| 3/3 [00:08<00:00,  2.86s/it]

The problem is when I add the second grader, eve. The process still works fine, and eve is in fact added to the reviewing team, but she is missing from the output:

$ repobee -p gitea -p ~/currentwork/repobee/src/_repobee/ext/reviewothers.py \
    reviews assign -a task-2 --sf students.txt -u dmusicant \
    --bu https://gitea.repobee.org/api/v1 -o dave-template --to dave-template \
    -t  ...  --grader eve
[WARNING] The Gitea plugin is in very early development, use at your own risk!                           
Fetching teams and repos: 100%|████████████████████████████████████████████| 3/3 [00:04<00:00,  1.57s/it]
Allocating reviews                                                                                       
Assigning jane to review alice-task-2                                                                    
Assigning jane to review bob-task-2                                                                      
Assigning jane to review ellen-task-2               
Creating review teams: 100%|████████████████

But when I run the same command again, then eve also appears in the output:

$ repobee -p gitea -p ~/currentwork/repobee/src/_repobee/ext/reviewothers.py \
    reviews assign -a task-2 --sf students.txt -u dmusicant \
    --bu https://gitea.repobee.org/api/v1 -o dave-template --to dave-template \
    -t  ...  --grader eve
[WARNING] The Gitea plugin is in very early development, use at your own risk!
Fetching teams and repos: 100%|████████████████████████████████████████████| 3/3 [00:04<00:00,  1.55s/it]
Allocating reviews
Assigning eve and jane to review alice-task-2       
Assigning eve and jane to review bob-task-2         
Assigning eve and jane to review ellen-task-2       
Creating review teams: 100%|███████████████████████████████████████████████| 3/3 [00:06<00:00,  2.15s/it]

I've started going through the code to try to understand why this delay might be occurring, but I also thought you just might know right away. Any thoughts?

[slarse]

Hi @dmusican, nice to hear from you again :)

I think I know what the problem is. When RepoBee assigns reviews, it first tries to fetch review teams, and if they don't exist it creates them. In the case where it fetches an existing review team and adds a member to it (as is the case with your second invocation), the fetched platform team is in fact not updated (this is by design, local platform API representations are not automatically synchronized).

Actually, I think the problem is right here, in create_teams:

repobee/src/_repobee/command/teams.py

Lines 35 to 39 in f2346a6

	# FIXME the returned team won't have the correct members if any new
	# ones are added. This should be fixed by disconnecting members
	# from teams, and having an api call "get_team_members"
	yield team

which is called from the review assignment command:

repobee/src/_repobee/command/peer.py

Lines 129 to 131 in f2346a6

	review_teams = _repobee.command.teams.create_teams(
	review_team_specs, plug.TeamPermission.PULL, api
	)

So, yeah, I'm 99% sure this is the cause, but I haven't verified it.

[dmusican]

Thanks, @slarse , that make sense. I spent a while looking at this, and laughed out loud when I saw the FIXME in there. Rebuilding that API is beyond the scope of what I'm trying to do here. :)

I think my choices are:

• live with it for the time being (and thus perhaps dump out a warning from the plugin)
• try to synchronize the local team with the platform one first (clearly, it's getting synchronized somewhere afterwards, since the output lags by one run)
• try to use the platform team directly. Any recommendations?

[slarse]

clearly, it's getting synchronized somewhere afterwards, since the output lags by one run

In the first run, the team is fetched before adding members to it, and only the members that were initially there are shown in the fetched team. Adding members to it on the platform does not update the local representation. In the second run, the team is fetched again (because it's an independent run) and the members assigned in the first run are thus correctly set in the second one.

To be clear, this is only a problem when adding members to an existing team. In the case of creating a new review team, the problem does not appear as then the members are added as part of the API request to create a team, and all members are thus correctly set in the first fetch.

Therefore, one quick workaround for you would be to just run the command once, and have the grader option be a list instead (such that you can supply --grader studentA studentB studentC).

On the note of that CLI option. As this is a command extension, and the options intermingle with ones from RepoBee and other plugins, I strongly recommend prefixing the --grader option with the name of the plugin to avoid option name collisions with other plugins or RepoBee itself. That is to say, I'd name it --reviewothers-grader (i.e. the variable should be called reviewothers_grader). See the very end of this section of the docs.

Rebuilding that API is beyond the scope of what I'm trying to do here. :)

Yeah, definitely. There is an easy solution without redesigning the API, and that is to query the platform for the team after having updated it in create_teams. That comes with a 30% performance overhead, though, as it would require an extra platform API request. But it might be worth it, as this behavior is confusing. WDYT?

try to use the platform team directly. Any recommendations?

This breaks multiplatform compatibility, so I would advise not to do this. The problem you're experiencing is a problem in RepoBee core, and thus that's where it should be fixed.

Also, note that the fact that the local representation is wrong doesn't actually matter in your case: the reviews are assigned correctly anyway. It's just a bit confusing from the user perspective.

[dmusican]

Thanks, this is helpful. Changing the name of the CLI option: that's easy, I've done that.

I like the idea of making the reviewothers-grader option be a list. If I do that, do the multiple students have to be in quotes or escaped somehow? If I try it as --grader studentA studentB studentC, I get an error that says repobee: error: unrecognized arguments: studentB studentC. Quoting the list, such as --grader "studentA studentB studentC" solves that particular problem, and does in fact turn them into a list... but since your example didn't have quotes, I wanted to make sure I wasn't missing something. I thought this might have to do with using a converter, but I don't think it gets to that stage.

[slarse]

Ah, it appears as if I haven't actually added dedicated support for creating list-like option. You need to pass the nargs keyword argument to the underlying argparse library. Here's an example of creating a list-like option that takes 1 or more arguments: https://github.com/repobee/repobee-csvgrades/blob/30663f559a6668e4f76e61ea5256d60cf4f1c656/repobee_csvgrades/csvgrades.py#L122

Putting quotes around the different arguments make them all one argument, so that's not quote the way to go. While you could of course put quotes around the arguments and then use a converter to split them into a list, it's better to let argparse to the heavy lifting :)

[dmusican]

Great, this seems like a good way to go. I've started trying to work with this example, but haven't yet succeeded in making it work. (I'm getting a "string indices must be integers" error.) In the csvgrades example, how/when does the _parse_teachers method get registered, or called? I think, but am not sure, that the analogous method I'm creating isn't getting found. (Regardless, I don't want to code by copying templates, I'd rather understand what this is doing.)

[slarse]

Haha, sorry, _parse_teachers is dead code! It's a remnant from before list-like options were supported in the config file.

There's no additional work required on your end, if you supply argparse_kwargs={"nargs": "+"}, then argparse will handle parsing to a list. So, assuming that --graders studentA studentB studentC, then you should get the list ["studentA", "studentB", "studentC"] into the self.graders variable.

[dmusican]

Thanks! It's working. :)

I can put together a PR, but before I do: where do you want this code to live? In the ext directory with the other plugins? Or externally, like the csvgrades plugin? Or entirely externally, i.e. in my own repo?

[slarse]

I can put together a PR, but before I do: where do you want this code to live? In the ext directory with the other plugins? Or externally, like the csvgrades plugin? Or entirely externally, i.e. in my own repo?

That's a good question. There are pros and cons to all approaches.

1. Merging it into RepoBee core (this repo)
   • Pros: Less maintenance effort overall, easier to access by users
   • Cons: We commit to having this in RepoBee, bloat for those that don't need it, more maintenance for me if you drop off the face of the earth :)
2. Making it an unofficial, external plugin (your own repo)
   • Pros: zero risk for me, it's "your" thing to maintain as you wish, no bloat in core
   • Cons: Significantly less discoverable by other users, possibly not as gratifying for you, more maintenance overall (needs a separate test suite)
3. Making it an official, external plugin (in the repobee org)
   • Pros: Still pretty much "your" thing, if unmaintained I can discontinue it, no bloat in core
   • Cons: Slightly less convenient for user to use (must be installed), more maintenance overall (needs a separate test suite)

Your thoughts on this? On my end, I'm undecided. I would prefer either merging into core or making it an official external plugin. I quite like the concept, and it's fun to have more contributors to the official feature set!

[dmusican]

My guess is that official external plugin makes sense; this way there's less pressure to make it perfect, and as you say, if I get distracted with other things and someday disappear you can always just give up on it.

I haven't worked on the test suite yet; I need to do that. Should I assume the easiest place to start would be to look at the test suite for one of the other external plugins?

[slarse]

My guess is that official external plugin makes sense; this way there's less pressure to make it perfect, and as you say, if I get distracted with other things and someday disappear you can always just give up on it.

Let's do that then! What would you like it to be called?

I haven't worked on the test suite yet; I need to do that. Should I assume the easiest place to start would be to look at the test suite for one of the other external plugins?

That, and the Packaging plugins guide in the docs. It contains instructions for how to create a standalone plugin, and generates a (very) rudimentary test suite for you.

I've created repobee-reviewothers and invited you as a maintainer of the repo. Let me know if you need any help!

[slarse]

P.s. if you're not happy with the name, we can change it later, I just picket the name of your plugin file.

[dmusican]

Great, I've accepted the invite -- I'll work on getting the thing tested and packaged.

[dmusican]

I may not be the fastest developer in town, but I've got the plugin packaged, with some rudimentary tests running, and pushed. Thanks for all of your help! I think we're ready to close this issue, as any further issues should presumably be recorded over at the repobee-reviewothers repo. Does all look good on your end?

[slarse]

Hi @dmusican,

Very cool! I'm a bit busy with prepping for ITiCSE 2021 right now, but most likely I'll have time to look over the plugin sometime next week. On a quick glance, the first order of affairs is to setup some CI, which is most easily done with GitHub actions. I can put in a PR for that.

But indeed, we can close this issue now and take any further discussion over on the reviewothers issue tracker :)