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

Open formgrader with a local configuration file #1859

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

Conversation

brichet
Copy link
Contributor

@brichet brichet commented Jan 26, 2024

This PR aims to open formgrader with a configuration file (nbgrader_config.py) from the local directory.

Currently, the formgrader loads the nbgrader_config.py files from:

  • the directories in jupyter_config_path
  • the directory where Jupyter lab started: os.cwd()
  • the directory CourseDirectory.root (which can be configured by one of the previous files)

With this PR, it will be possible to start the Formgrader with a config file in the curent directory in lab filebrowser, by using the new menu entry Formgrader (local).
It will reinitialize the FormgradeExtension object with the additional configuration of the current directory.
In this case, the configuration file existing in the root directory of Jupyter lab app will not been loaded.

Using the classic menu entry Formgrader will restore the FormgradeExtension object with the initial configuration.

This feature is not enabled by default, and must be set by user in settings panel (or user settings file):

Peek 2024-01-26 11-38

This PR also adds a command line configuration to display the current config in the formgrader UI: --FormgradeExtension.debug=true.
This can be helpful to test this PR (at least).

I tested it on a local jupyterlab server and using jupyterhub in docker (demo multiple classes), but I think it must be tested in some real environments to ensure it does not break any existing behavior.

** CAUTION **

  • it is probably not usable if several users access the same jupyterlab server, as it modifies the FormgradeExtension object on the server.
  • if one of the config file provides a CourseDirectory.root configuration (which is the case on jupyterhub AFAIK), this parameter must be overwritten in the local config file. This is intentional, we don't silently remove certain parameters from the loaded files, we just change the files that are loaded.

cc. @nthiery

Copy link
Contributor

Binder 👈 Launch a Binder on branch brichet/nbgrader/local_formgrader

@brichet brichet self-assigned this Jan 26, 2024
@nthiery
Copy link
Contributor

nthiery commented Jan 26, 2024

Excellent! Our next project to grade is on February the 9th. So I have to
test it seriously before that, and we will have more massive testing then :-)

@nthiery
Copy link
Contributor

nthiery commented Feb 1, 2024

Tiny update: the debug flag is actually --FormgradeExtension.debug=true

@nthiery
Copy link
Contributor

nthiery commented Feb 1, 2024

I just tried this branch on my machine. In my home directory ~/, I have a subdirectory ~/C
for one of my courses. I

  • launched jupyter lab from my home directory using --FormgradeExtension.debug=True,
  • navigated to ~/C in jupyterlab's file navigator
  • activated the feature in the settings panel
  • opened the local formgrader
  • checked the nbgrader config
  • navigated around in the formgraded.

All of this went smoothly :-)

However, when going to Grade manually -> AssignmentXX, I get 'No data available in table'. Whereas if I launch jupyter lab from ~/C, and do the same with the formgrader or local form grader, I do see my student's submissions.

For the record, another difference is that the current configuration is displayed in a long single line in the second case, and properly line wrapped and indented in the first case.

I am not sure how to proceed to debug further.

In case this can help, I am available tomorrow starting from 10:30 for live debugging.

Cheers,

@brichet
Copy link
Contributor Author

brichet commented Feb 2, 2024

Tiny update: the debug flag is actually --FormgradeExtension.debug=true

Thanks, I updated the description.

@nthiery
Copy link
Contributor

nthiery commented Feb 10, 2024

Ok, I found the issue and a workaround. In my nbgrader_config.py,
the path of the gradebook is configured relatively to the current directory:

c.CourseDirectory.db_url = 'sqlite:///.gradebook.db'

However the formgrader apparently fetches the gradebook from ~/.gradebook
instead of ~/C/.gradebook. If instead I set:

c.CourseDirectory.db_url = 'sqlite:///C/.gradebook.db'

then I can see my student's submissions.

@nthiery
Copy link
Contributor

nthiery commented Feb 10, 2024

Another issue: if RTC is enabled, then the local formgrader seems to be opened with
the URL

http://localhost:8888/formgrader?path=RTC C

Forcing the url:

http://localhost:8888/formgrader?path=C

works around this.

@nthiery
Copy link
Contributor

nthiery commented Feb 10, 2024

Aside from this, it seems at this stage completely functional. I'll grade
my assignment and report again in case I stumble on anything else.

@nthiery
Copy link
Contributor

nthiery commented Feb 15, 2024

Hello @brichet,

After experimenting, I confirm that the workaround we discussed this morning about paths relative to the course root in db_url works. For the record, here is what I inserted in my nbgrader_config.py:

import os
server_root = os.getcwd()
course_root = os.path.dirname(__file__)
course_root = os.path.relpath(course_root, server_root)
c.CourseDirectory.db_url = f'sqlite:///{course_root}/.gradebook.db'

Ideally, we could make the idiom into:

c.CourseDirectory.db_url = f'sqlite:///{c.CourseDirectory.root}/.gradebook.db'

And then it would only be a question of documenting it together with
the feature of opening locally the formgrader.

@brichet
Copy link
Contributor Author

brichet commented Feb 20, 2024

@nthiery I haven't found a way to retrieve the config from the config file.
The Config object c seems to be always empty when the config file is executed. I don't know if this is expected or not.

@nthiery
Copy link
Contributor

nthiery commented Feb 22, 2024

Ok, too bad. Thanks for trying out!

For the record, the trick above worked smoothly while I graded a bunch of
notebooks. So, at this stage, I suggest to:

  • Document the trick and get this ticket merged.
  • Think about another way of having a short hand, in a potential follow up ticket.

@brichet
Copy link
Contributor Author

brichet commented Mar 4, 2024

Failures seems to be related to incompatibility between pytest 8.1.0 and nbval 0.10.0
computationalmodelling/nbval#202
computationalmodelling/nbval#203

@nthiery
Copy link
Contributor

nthiery commented Mar 12, 2024

Hi @brichet,

For the record: in fact absolute paths work for the gradebook. This is a bit simpler, and in fact
slightly more robust, in case the course root is not a subdirectory of the server root. Yes, I thought
this could not happen, but I actually encountered this today ... probably a bug of our server. Anyway,
here is the updated trick for the nbgraderconfig.py:

import os
course_root = os.path.dirname(__file__)
c.CourseDirectory.db_url = f'sqlite:///{course_root}/.gradebook.db'

@nthiery
Copy link
Contributor

nthiery commented Mar 12, 2024

I don't have something solid to report yet, but it seems that there is some hidden state somewhere. If a local formgrader is loaded once with formgrader?path=foo, opening it again with formgrader?path=bar still reloads the configuration from foo. It might even persist across sessions (???).

We have a grading session this afternoon, so I'll try to use the occasion to analyse further. Let me know if you manage to reproduce it.

@brichet
Copy link
Contributor Author

brichet commented Mar 14, 2024

I don't have something solid to report yet, but it seems that there is some hidden state somewhere. If a local formgrader is loaded once with formgrader?path=foo, opening it again with formgrader?path=bar still reloads the configuration from foo. It might even persist across sessions (???).

We have a grading session this afternoon, so I'll try to use the occasion to analyse further. Let me know if you manage to reproduce it.

@nthiery I can't reproduce it.
Were you able to reproduce it ? If so, are there any error messages in the terminal ?

@nthiery
Copy link
Contributor

nthiery commented Mar 14, 2024

No haven't been able to reproduce it lately. So I'll assume it was related to our server having
a misconfigured server root. Sorry for the noise and thanks for checking!

@nthiery nthiery self-requested a review March 17, 2024 09:08
Copy link
Contributor

@nthiery nthiery left a comment

Choose a reason for hiding this comment

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

Some feedback after using the local formgrader on the field.

It has been working smoothly and resolves as hoped for the motivating
use case: we can now easily have multiple nbgrader courses
configuration on the same infrastructure, e.g. Jupyterhub.

That's really cool. Thank you @brichet!

The procedure to access the feature through the UI is a bit complex:
go to the settings; find nbgrader; activate the local formgrader; find
Local formgrader in the UI. At the end, it was simpler to just tell
collaborators to open the URL /formgrader?path=.... Also, the
standard UI in JupyterLab to run an application in a local directory
is to do it from the launcher; not from a global menu.

Suggestions:

  • Scrap the "local formgrader" item in the global menu and the
    configuration option to activate it.
  • Enable the ?path=... also for subpaths of the formgrader. E.g.
    /formgrader/grading/?path=...
  • Upon opening a local formgrader on a specified path, test whether an
    existing formgrader is already open with a different path (is that
    possible?). If yes, abort with a message "please close the other
    running formgraders".
  • Scrap the optional command line option
    "--FormgradeExtension.debug=true. Instead, always display a line in
    the formgrader "Formgrader with configuration taken from directory
    xxx (click to expand)". It's indeed always useful for the user to
    have a visual clue on where the formgrader is running, and it grants
    easy access to debugging information. We should presumably not worry
    too much about vertical space at this stage; anyway at some point
    there will be a UI overhaul which will be the occasion to optimize
    it. Let's just make sure relevant information is easily accessible.

With that, I believe this PR would be good to go from the user's perspective.

UI suggestion for this or a later PR:

  • Add a new activity "Local formgrader" in the launcher, with a
    tooltip "Launch nbgrader's formgrader, with configuration taken from
    this directory".

@brichet
Copy link
Contributor Author

brichet commented Mar 19, 2024

Thanks for the feedback @nthiery.

  • Scrap the "local formgrader" item in the global menu

What is the motivation to remove it?
It is only a shortcut to avoid using the URL, and it explicitly shows that we can open formgrader with different settings.

and the configuration option to activate it.

I like the idea of keeping it intentional, as it changes the server configuration when used.
This setting should not be used if several users are using on the same Jupyterlab server.
Maybe some feedback from other usage of nbgrader could help.

  • Enable the ?path=... also for subpaths of the formgrader. E.g.
    /formgrader/grading/?path=...

I'll see if there is an easy way to handle it.

  • Scrap the optional command line option
    "--FormgradeExtension.debug=true

Let's set it to true by default, so that people can disable it.

@nthiery
Copy link
Contributor

nthiery commented Mar 20, 2024

motivation to remove it

That's with the alternate UI (activity in the launcher) in mind to avoid having two UI's for the same action.
It could be left there until the alternate UI is implemented, but then users will learn it and have to unlearn it.

several users on the same Jupyterlab server

To make sure I get the technical details right: you could actually have two users use the local
formgrader on the same JupyterLab server; but only if this is in the same local directory, right?
That there are one or several users does not make a difference, except of course that it's easier
to coordinate with just yourself than with someone else :-)

It seems to me that launching the formgrader from the URL with a path argument, or through
the activity in the launcher, is quite intentional. Of course, that's only reasonable if there can
be some security check, with an error if opening a formgrader while another formgrader is
already in use with another path. For the "activity in the launcher" UI, it would be nice if the
activity would be grayed out if there is another formgrader running with a different path.

Optional command line option with default to true

Yes, that works too.

@brichet
Copy link
Contributor Author

brichet commented Mar 20, 2024

To make sure I get the technical details right: you could actually have two users use the local
formgrader on the same JupyterLab server; but only if this is in the same local directory, right?

This is right. Indeed, if the users open formgrader in the same directory it will work.
But there is only one Formgrader object on the server side, which is now configured on the fly.
This is more flexible, but also less safe, depending on the use.

Let's take an example:

  • User1 opens formgrader from directory A → Formgrader object is configured according to nbgrader_config.py in directory A
  • User2 opens formgrader globally (or from directory B) → Formgrader object is configured according to global nbgrader_config.py file (or the one in directory B)
  • User1 also get the global config (or the directory B one) without knowing. Behavior will be rather unstable for him.

How to fix it ?

We could manage several Formgrader objects on the server (probably one per user, or one per config).
Or we could manage the current connected (and active) clients, to disallow users to open an other configuration if one is in use (as you proposed IIUC).

I'd advocate the first solution, but it will require (much?) more work, and could be done in a later PR.
As it stands, it's probably safer to keep it as optional, and add a warning about using it with multiple users on the same Jupyterlab instance.

@nthiery
Copy link
Contributor

nthiery commented Mar 21, 2024

Thanks for the confirmation!

Agreed the first solution is definitely the proper solution and we should eventually have it. Presumably the proper granularity is one configuration object per directory on the server (with lazyness: the configuration is only created if a formagrader is open with that directory); I guess that's what you meant by "one per config".

Agreed also that the current solution is just temporary, so it's not worth investing a lot of energy polishing/hardening it, let alone discussing it :-) Also, presumably it will be a rare event that people will use local configuration on a shared jupyter server. With that in mind, I let you finalize this PR as you see fit. If some of the hardening actions (like warning against already running sessions with different config) are trivial to implement, that's great. Otherwise that's fine too!

…b instance, and show the configuration by default in formgrader
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.

None yet

2 participants