-
Notifications
You must be signed in to change notification settings - Fork 317
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
Conversation
calling Jupyterhub API with user token requires Jupyterhub 0.8 |
a288c9b
to
0680787
Compare
0680787
to
3256d58
Compare
nbgrader/exchange/exchange.py
Outdated
courses = set() | ||
for group in groups: | ||
if group.startswith('nbgrader-') or group.startswith('formgrade-'): | ||
courses.add('-'.join(group.split('-')[1:])) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
nbgrader/exchange/exchange.py
Outdated
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.") |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
10be8f4
to
df49640
Compare
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 |
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. |
Thanks for working on this! Apologies for not being able to look at this
yet—I’ve been on vacation the last few weeks without a computer and haven’t
been able to attend to nbgrader issues in the meantime. But, I’ll be back
tomorrow and can look at this further soon—I should be able to provide some
feedback later this week!
…On Mon, Oct 9, 2017 at 3:30 PM Andrea Zonca ***@***.***> wrote:
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.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#893 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAFF9IeLR1F2ESvJmc0MXwTU0R2uUs9Qks5sqe6-gaJpZM4PaOww>
.
|
thanks @jhamrick no hurry, even next week is fine! |
There was a problem hiding this 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.
nbgrader/exchange/exchange.py
Outdated
user = os.environ['JUPYTERHUB_USER'] | ||
else: | ||
self.exit("JUPYTERHUB_USER env is required to run the exchange features of nbgrader.") | ||
import json |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
nbgrader/exchange/exchange.py
Outdated
"""Check if student is enrolled in course""" | ||
from tornado.httpclient import HTTPClient, HTTPRequest | ||
|
||
if os.getenv('JUPYTERHUB_API_TOKEN'): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
nbgrader/exchange/exchange.py
Outdated
groups = json.loads(resp.body.decode('utf8', 'replace'))['groups'] | ||
courses = set() | ||
for group in groups: | ||
if group.startswith('nbgrader-') or group.startswith('formgrade-'): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
I have implemented storing the 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. |
Here an example on how this works,
The student is not enrolled, they cannot see it:
Same in the extension: Then we add the student to the class:
Now the student can access the assignments:
and |
@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.
Maybe @minrk has suggestions? |
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 |
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? |
@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 ? |
No, I tested with only with the nbgrader extension
Il mar 16 gen 2018, 15:33 Sigurður Baldursson <notifications@github.com> ha
scritto:
… @zonca <https://github.com/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 ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#893 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAXYcvk38cgn3w04zfqexHMWeEzqnvNdks5tLLM7gaJpZM4PaOww>
.
|
I had some time to test the multiple courses branch from zonca so I merged it with the current master(0.6.0dev) Issue: Forbidden api tokenWhen 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). 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. |
@sigurdurb nice! I wonder if you can use the browser debugger to check what is going on inside
|
My test setup:
@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. I am running this as a managed service(starting everything at once)
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, And gets called from: 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 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 ? |
@sigurdurb sure, |
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. |
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 |
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 I am going to go ahead and close this PR since the other one is now merged. |
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_testAdd to the documentation
nbgrader-
for students andformgrade-
for instructors