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

Setup nbgrader in one class, one grader mode #155

Open
wants to merge 73 commits into
base: main
Choose a base branch
from
Open

Setup nbgrader in one class, one grader mode #155

wants to merge 73 commits into from

Conversation

sverhoeven
Copy link
Member

@sverhoeven sverhoeven commented Feb 13, 2024

For testing locally please upgrade vagrant to latest version.

Uses https://github.com/eWaterCycle/teaching/tree/nbgrader-quickstart as course material source.

This PR also includes some linting and deprecation fixes.

…terhub_config.py in `systemctl status jupyterhub`
ERROR! [DEPRECATED]: ansible.builtin.include has been removed. Use include_tasks or import_tasks instead. This feature was removed from ansible-core in a release after 2023-05-16. Please update your playbooks.
Ansible failed to complete successfully. Any error output should be
visible above. Please fix these errors and try again.
@sverhoeven sverhoeven mentioned this pull request Feb 14, 2024
@sverhoeven sverhoeven marked this pull request as ready for review February 14, 2024 13:02
Use value from defaults
Copy link
Collaborator

@Peter9192 Peter9192 left a comment

Choose a reason for hiding this comment

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

Very cool stuff! I managed to start up a local infra with vagrant and become my own teacher 😎 Approving as it works and is ready to use, but you may still consider the few comments I had at your own discretion.

  • As a student I don't have permission to see the contents of /var/local/nbgrader/exchange/course1/inbound/, as a teacher I do. This seems good.
  • As a student, I am able to open the formgrader, which, even if it's not a problem, I found confusing, to say the least.
  • As a student, I also have the right hand side tab to "create assignment", which doesn't really make sense either, or does it?
  • Have you considered moving all the teaching-specific config from README.md to TEACH.md?

License
-------

BSD
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was wondering where this is documented. Is it standard practice to follow the license of the main third-party software?

README.md Outdated
The value should be a JSON string in format of `[["username1", "password1"]]`.
- Add `course_repo` parameter with git repository url of the course source material.
Use default value of `https://github.com/eWaterCycle/teaching.git`
- Add `course_version` paramneter with the version,branch or tag of the course repository to use.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Add `course_version` paramneter with the version,branch or tag of the course repository to use.
- Add `course_version` parameter with the version, branch or tag of the course repository to use.

TEACH.md Outdated
@@ -0,0 +1,28 @@
# Teaching with eWaterCycle infra
Copy link
Collaborator

Choose a reason for hiding this comment

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

Refer to this file from README?
Also, the readme already contains teaching-specific instructions, might that fit better here?

README.md Outdated
Comment on lines 50 to 52
# Vagrant user is instructor
# The students defined below can be used to login as a student
students: '[["student1", "student1"], ["student2", "student2"]]'
Copy link
Collaborator

@Peter9192 Peter9192 Feb 16, 2024

Choose a reason for hiding this comment

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

Now the readme contains teaching specific instructions. For me it's okay to have two versions of README, one for standard and one for teaching. But perhaps we should add a note here that these instructions are specifically for teaching, and if you want standard infra, you should check out the other branch (and vice versa)?

TEACH.md Outdated
Comment on lines 7 to 8
During creation you can set the `students` parameter to create local posix accounts for students.
The value for the `students` parameter can be generated by running [create_student_passwords.py](create_student_passwords.py) script with list of username (one on each line) as input. See docs in script for more details. The passwords generated by the script should be distributed to the students.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
During creation you can set the `students` parameter to create local posix accounts for students.
The value for the `students` parameter can be generated by running [create_student_passwords.py](create_student_passwords.py) script with list of username (one on each line) as input. See docs in script for more details. The passwords generated by the script should be distributed to the students.
During creation you can set the `students` parameter to create local posix accounts for students.
The value for the `students` parameter is a list of [student, password] values. You can use the python script [create_student_passwords.py](create_student_passwords.py) to generate passwords. To use it, create a file "usernames.txt" with one username on each line. Then call the script to generate passwords. They will be stored in a new file called `students.json`. See docs in script for more details. The passwords generated by the script should be distributed to the students.

Comment on lines 58 to 67
- name: Add students to course
when: students is defined
ansible.builtin.command: '{{ conda_environment_bin }}/nbgrader db student add "{{ item[0] }}"'
environment:
PATH: "{{ conda_environment_bin }}:{{ ansible_env.PATH }}"
loop: "{{ students | from_json }}"
args:
chdir: "/home/{{ grader_user }}/{{ course_id }}"
become: true
become_user: "{{ grader_user }}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are new student accounts that are added e.g. through sram automatically added to the course?

Copy link
Member Author

Choose a reason for hiding this comment

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

This task is runs once during provisioning.

I could add add command to user sync cron job script , but that will be very flaky.

So I would rather wait for the lms integration where we will fetch usernames from lms and create them in VM.

@BSchilperoort
Copy link
Member

Setup seems to work for the lesson development. Hopefully also when the students access it 🤞

We discussed limiting the students on their memory & cpu use to reduce the chance of a single student locking up a server. That could still be nice to add.

@sverhoeven sverhoeven mentioned this pull request Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants