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

[WIP] Support for multiple classes via Jupyterhub groups #893

Closed
wants to merge 22 commits into from

Conversation

zonca
Copy link
Contributor

@zonca zonca commented Sep 17, 2017

Exploring the implementation of support for multiple classes via Jupyterhub groups proposed in #544.

For now only restricts the dropdown of the Assignments List nbextension based on the membership of a student to Jupyterhub groups.

I prepared a ansible setup with users and groups preconfigured to test this at: https://github.com/zonca/jupyterhub-deploy-teaching/tree/nbgrader_test

Add to the documentation

  • general description on how Jupyterhub groups are used by nbgrader
  • groups need to be named nbgrader- for students and formgrade- for instructors
  • Jupyterhub > 0.8 required

@zonca
Copy link
Contributor Author

zonca commented Sep 19, 2017

calling Jupyterhub API with user token requires Jupyterhub 0.8

courses = set()
for group in groups:
if group.startswith('nbgrader-') or group.startswith('formgrade-'):
courses.add('-'.join(group.split('-')[1:]))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here I assume that students are in the nbgrader-course_id Jupyterhub group and the instructors in formgrade-course_id group, is this good enough?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could do:

courses.add(group.split('-', 1)[1])

to get everything after the first - without needing to re-join.

if os.getenv('JUPYTERHUB_API_TOKEN'):
api_token = os.environ['JUPYTERHUB_API_TOKEN']
else:
self.exit("JUPYTERHUB_API_TOKEN env is required to run the exchange features of nbgrader.")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@minrk can you please take a look if I'm using the APIs correctly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use of the API looks absolutely right to me.

@zonca
Copy link
Contributor Author

zonca commented Sep 24, 2017

The required functionality of the Exchange is implemented. Tests needs to be updated and for that I need to Mock the Jupyterhub APIs.

However, most importantly, I need to add functionality to the add_student and remove_student methods of the Gradebook object so that when students are added or removed from a course database they are also added or removed from the Jupyterhub group (only if it detects we are running inside Jupyterhub).
So I was thinking to add course_id to the constructor of Gradebook and have it stored inside the gradebook.db database (when created in nbgrader/apps/api.py), then used to talk to the Jupyterhub APIs to manage the groups of the users.

@jhamrick @lgpage do you think this is suitable?

@zonca
Copy link
Contributor Author

zonca commented Oct 9, 2017

does anybody have feedback if this way of implementing multiple classes in the assignment list extension is ok?

If not I'll try to go ahead as it is and try to have a complete working version.

@jhamrick
Copy link
Member

jhamrick commented Oct 9, 2017 via email

@zonca
Copy link
Contributor Author

zonca commented Oct 9, 2017

thanks @jhamrick no hurry, even next week is fine!

Copy link
Member

@jhamrick jhamrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks great! I think this way of implementing the multiple classes should be fine.

I like the idea of adding students to the jupyterhub group when they're added to the course database. It should be fine to add the course id as an argument to the constructor of the gradebook, though I would make it an optional argument---some people use nbgrader for a single class without JupyterHub, and we shouldn't force them to have to specify the course id if they're not using that functionality currently.

user = os.environ['JUPYTERHUB_USER']
else:
self.exit("JUPYTERHUB_USER env is required to run the exchange features of nbgrader.")
import json
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably would be better to have this import at the top of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

"""Check if student is enrolled in course"""
from tornado.httpclient import HTTPClient, HTTPRequest

if os.getenv('JUPYTERHUB_API_TOKEN'):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So just to clarify, this is an environment variable that's automatically set by JupyterHub, even in new terminal environments? So if I launch a terminal from the notebook server and try to run nbgrader list, will that still work appropriately?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, correct, but just on Jupyterhub >= 0.8, this feature of nbgrader would not work on 0.7.2

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, good to know, we should probably mention that in the docs too.

groups = json.loads(resp.body.decode('utf8', 'replace'))['groups']
courses = set()
for group in groups:
if group.startswith('nbgrader-') or group.startswith('formgrade-'):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we're basically making a strong assumption here that all groups that restrict access to nbgrader courses will be prefixed with nbgrader- or formgrade-. That's fine, I just am noting this so it can be added to the documentation here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I added a list of items to add to the docs in the top post

@@ -228,7 +228,10 @@ class CourseListHandler(BaseAssignmentHandler):

@web.authenticated
def get(self):
self.finish(json.dumps(self.manager.list_courses()))
courses_list = self.manager.list_courses()
if 'value' in courses_list:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should check the value of the "success" key. If it's false then the value will be a traceback rather than a list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, added the check as if courses_list['success'] and 'value' in courses_list:

@zonca
Copy link
Contributor Author

zonca commented Nov 2, 2017

I have implemented storing the course_id into the Gradebook database. I had to create a new base class in the dbapp classes to create an Exchange object and extract the course_id from it. Now the first time that the db is created from any of the nbgrader db commands, it includes course_id.

Next I'll implement the actual calls to Jupyterhub to set the users, I have the impression that I might not need to store the course_id into the db but read from the configuration file as I am doing now for initializing correctly the Gradebook. I'll find out soon and refactor if needed.

@zonca
Copy link
Contributor Author

zonca commented Nov 13, 2017

Here an example on how this works,

instructor2 releases an assignment:

instructor2@132-249-238-87:~/nbgrader/course2$ nbgrader release course2assign1
[ReleaseApp | INFO] Source: /home/instructor2/nbgrader/course2/release/./course2assign1
[ReleaseApp | INFO] Destination: /srv/nbgrader/exchange/course2/outbound/course2assign1
[ReleaseApp | INFO] Released as: course2 course2assign1

The student is not enrolled, they cannot see it:

student2-course2@132-249-238-87:~$ nbgrader list
[ListApp | WARNING] No nbgrader_config.py file found (rerun with --debug to see where nbgrader is looking)
Call Jupyterhub API /users/student2-course2 post_data: None
[ListApp | INFO] Released assignments:

Same in the extension:

screenshot 2017-11-13 at 16 04 59

Then we add the student to the class:

instructor2@132-249-238-87:~/nbgrader/course2$ nbgrader db student add student2-course2
[DbStudentAddApp | INFO] Creating/updating student with ID 'student2-course2': {'last_name': None, 'email': None, 'first_name': None
}
Call Jupyterhub API /groups/nbgrader-course2 post_data: None
Call Jupyterhub API /groups/nbgrader-course2/users post_data: {'users': ['student2-course2']}
Student student2-course2 added to Jupyterhub group nbgrader-course2

Now the student can access the assignments:

student2-course2@132-249-238-87:~$ nbgrader list
[ListApp | WARNING] No nbgrader_config.py file found (rerun with --debug to see where nbgrader is looking)
Call Jupyterhub API /users/student2-course2 post_data: None
[ListApp | INFO] Released assignments:
[ListApp | INFO] course2 course2assign1

and

screenshot 2017-11-13 at 16 19 28

@zonca
Copy link
Contributor Author

zonca commented Nov 13, 2017

@jhamrick I did more testing, I think the implementation is complete. However implement testing is tough. In order to test this functionality I need somehow to mock the calls to the Jupyterhub API.

  • First idea was to mock the function query_jupyterhub_api, however in the testing the nbserver is running in a separate process, so I don't see a workaround
  • Second would be to have some actual simple HTTP server that answers to the API calls "pretending to be Jupyterhub". The issue here is that nbgrader will try to authenticate...at this point the HTTP server becomes very complex...

Maybe @minrk has suggestions?

@jhamrick
Copy link
Member

jhamrick commented Dec 12, 2017

You could also launch JupyterHub in a separate process---this is actually what I did in the tests for a long time before I turned the formgrader into a notebook server extension. If you're interested in going down this road there is a lot of code you could reuse on the 0.4.x branch. Here is the class I wrote for launching JupyterHub in a separate process:

https://github.com/jupyter/nbgrader/blob/0.4.x/nbgrader/tests/formgrader/manager.py

And also a fake user authenticator and spawner:

https://github.com/jupyter/nbgrader/blob/0.4.x/nbgrader/tests/formgrader/fakeuser.py

@zonca
Copy link
Contributor Author

zonca commented Dec 22, 2017

thanks @jhamrick, it looks like it is going to be a significant effort that unfortunately I cannot make right now. Do you know if somebody else interested in this feature could take it from here and implement the testing part?

@sigurdurb
Copy link
Contributor

@zonca Great work, I saw above you tested it with nbgrader in the console, did you also test it through the Formgrader with Jupyterhub, and if so, did it work as expected ?

@zonca
Copy link
Contributor Author

zonca commented Jan 16, 2018 via email

@sigurdurb
Copy link
Contributor

I had some time to test the multiple courses branch from zonca so I merged it with the current master(0.6.0dev)
Really only found 2 issues, one with the formgrader and one that is more nbgrader

Issue: Forbidden api token

When I had an invalid api token which resulted in a forbidden status the nbgrader command line (nbgrader db student add {student_id}) still said it had added it to the jupyterhub group even though really it resulted in a forbidden status. We could simply make and raise an JupyterhubApiException before this line and except it then somewhere here

Issue: No Collect button and Release button does not change.

Collect button dissappears(tested the normal master vs. zonca branch).
After pushing Release it says it is successfull (but no collect button appears). Also the release button does not change into the unrelease X icon.
The assignment still shows up in the assignment list for the students though so this is more a Formgrader issue I guess.
Then the student can submit back to the exchange but the formgrader does not see the submission.
because he has no collect button ofcourse. ( Through the terminal nbgrader collect {assignmentname} works though, that might give some hint where the issue might be)

Thats really the only thing that I see conflicting now, otherwise the flow is very good. I would be happy to see this merged in the next version of nbgrader.
Any ideas where to start looking for a fix for the Issue with the collect and release button ? The logs are not helping nor the browser console. Maybe a similar issue has come up before where things buttons disappear?

@zonca
Copy link
Contributor Author

zonca commented Apr 30, 2018

@sigurdurb nice! I wonder if you can use the browser debugger to check what is going on inside manage_assignments.js:

@sigurdurb
Copy link
Contributor

My test setup:

  • Ubuntu 16.04
  • jupyterhub version 0.8.1
  • notebook version 5.4.1
  • jupyter version 4.4.0
  • nbgrader version 0.6.0.dev (merged with zoncas multiple_courses branch.)

@zonca It seems like the issue(No Collect button and Release button does not change) gets self–fixed when I have a valid api_token set when I start the service.
At first I thought it was a little bit weird because when I test it with just regular nbgrader I don't need to explicitly give it an api token for the buttons to appear and work. But that is only because the current master is not querying jupyterhub. Even if I take the JUPYTER_API_TOKEN that I get automatically and put it in a simple script to query the jupyterhub api it also results in a forbidden error. Probably the one given by default is just so that the user can query his own service and not jupyterhub.

I am running this as a managed service(starting everything at once)
So starting the service like this now with the api_token set works:

c.Jupyterhub.load_groups = {
     'formgrade-gagr':['sigurdb', 'gagr'],
     'nbgrader-gagr':[],
}
c.JupyterHub.services = [{'name':'gagr20181',
                          'url':'http://127.0.0.1:9993',
                          'api_token':'4d60feb9df9f480db1eb8b77a4ef81ad',
'command':['jupyterhub-singleuser',
           '--group=formgrade-gagr',
           '--debug',
          ],
        'user':'gagr',
        'cwd':'/home/gagr',
}]

I did not notice in my last commit but the logs say that a status of 403(forbidden) is returned from a call to /hub/api/users/<course_id> when I am in the formgrader,
That request that results in the forbidden status can be traced to
exchange.py

And gets called from:
list.py

Also in manage_assignments.js the reason for the icons/buttons not appearing is because every assignment is stuck in the "draft" status. that is because of the info['status'] = 'released' in list.py above
so this line in manage assignments only has draft status
and this line

Tldr: My suggestion would be to replace the print statement in exhange.py with self.log.error give a hint to set the api_token variable, what do you think ?

@zonca
Copy link
Contributor Author

zonca commented May 2, 2018

@sigurdurb sure,
also, isn't this triggered if the token is not available? https://github.com/jupyter/nbgrader/pull/893/files#diff-10a5f12d1812bb391fbe4c6298e232d8R58

@sigurdurb
Copy link
Contributor

sigurdurb commented May 2, 2018

In theory it should but JUPYTER_API_TOKEN is always there in my case, even if I try to set the api_token : None in jupyterhub_config I get an error that it should be a unicode string. Also if I set it as an empty string it automatically gets randomly generated. Also if I skip the api_token it gets automatically set. And like I said above; "Even if I take the JUPYTER_API_TOKEN that I get automatically and put it in a simple script to query the jupyterhub api it also results in a forbidden error. " I tried some more paths from here: https://jupyterhub.readthedocs.io/en/0.7.2/_static/rest-api/index.html but I always get Forbidden. So I am not sure why this JUPYTERHUB_API_TOKEN is always being set by default even though it doesnt work(maybe @minrk can tell us). I guess it makes sense to have it not working because even normal users get a JUPYTERHUB_API_TOKEN so it makes sense that you don't want them to have access to the Api to delete groups or whatever.

@meeseeksmachine
Copy link

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/jupyter-nbgrader-hackathon-topics/1040/1

@jhamrick
Copy link
Member

Thanks so much for all the work you put into this @zonca ! We got @sigurdurb 's PR merged (#1040) which built on top of this one, and are working in the multiple-classes-dev branch to add additional support for having multiple classes even beyond exposing the correct classes to students. I really appreciate the initial work you put into this and I think is going to be a nice foundation for the changes we are making during the Edinburgh hackathon 🙂

I am going to go ahead and close this PR since the other one is now merged.

@jhamrick jhamrick closed this May 29, 2019
@jhamrick jhamrick modified the milestones: 0.6.0, No action May 29, 2019
@jhamrick jhamrick mentioned this pull request Jun 1, 2019
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants