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

Specifying parameters to questions at course, instance, and assessment level #3848

Open
wants to merge 69 commits into
base: master
Choose a base branch
from

Conversation

nealterrell
Copy link
Contributor

@nealterrell nealterrell commented Feb 18, 2021

Addresses #3530.

This adds support for questionParams fields in the JSON files for courses, course instances, and assessments (and its zones, alternative groups, and assessment questions). By adding these fields to one of these objects, individual questions can have their data["params"] dictionary pre-filled with values specified by the assessment containing the question, or the course instance that the assessment is offered, or the course itself that owns the question.

Example motivation: in my CS1 course, we teach 12 weeks of Python and 3 weeks of Java. I have a question for identifying the Python primitive type to use for a given scenario description. I'd like to do the same type of skills practice when we move to Java, using the Java primitive type names instead. Currently I have to write two separate questions with the exact same logic; now, I can use questionParams to configure the question with a language field that is used in server.py to display different type names depending on the language.

For example, I can add "questionParams": {"language": "Python"} to infoCourse.json to set Python as the default language parameter; the server.py sees that in data["params"]["language"], then uses Python type names for the question:

image

When we later move to Java, I add "questionParams": {"language": "Java"} to my infoAssessment.json. (I can add it to the question object individually, or to a zone (to pass to all zone question), or to an alternatives group (likewise)). This value overrides the course parameter, so now when a student takes this assessment, the question changes to Java types:

image

Parameters are overridden in the order you would hope: Course < Course instance < Assessment < Zone < Alternatives group < Assessment qusetion.

Changes

  • Added questionParams fields to the info files for course, course instance, and assessment
  • Synced questionParams to database
  • When reading a question from database, merge question params from the available sources. (If only given a question ID, we can merge the course params; if given a question instance ID, merge with course, course instance, and assessment params).
  • Changed questions_select to return JSONB instead of questions.

TODO

  • write tests
  • add to exampleCourse?

@nealterrell nealterrell force-pushed the question-params-course branch 9 times, most recently from fd75889 to 3e4d0cf Compare February 24, 2021 21:04
@nealterrell
Copy link
Contributor Author

I believe this is ready for review.

@nealterrell
Copy link
Contributor Author

Hi @mwest1066, I will bring this PR up to date with main, and then I'd love to get some input on getting it merged!

docs/question.md Outdated Show resolved Hide resolved
docs/question.md Outdated Show resolved Hide resolved
docs/question.md Show resolved Hide resolved
migrations/245_pl_course__question_params__add.sql Outdated Show resolved Hide resolved
sprocs/sync_course_instances.sql Outdated Show resolved Hide resolved
tests/testQuestionParams.js Outdated Show resolved Hide resolved
tests/testQuestionParams.js Outdated Show resolved Hide resolved
tests/testQuestionParams.js Outdated Show resolved Hide resolved
tests/testQuestionParams.js Outdated Show resolved Hide resolved
docs/question.md Outdated Show resolved Hide resolved
docs/question.md Outdated Show resolved Hide resolved
docs/question.md Outdated Show resolved Hide resolved

A `questionParams` is a JSON object that can be placed in three different PrairieLearn file types. When a question's `generate()` function is called, the `data["params"]` dictionary will be pre-filled with the key-value pairs defined by `questionParams` in these locations:

1. `infoCourse.json`: parameters defined here will be passed to all questions in the course, including when they are previewed by an instructor without being attached to an assessment.
Copy link
Member

Choose a reason for hiding this comment

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

I'm concerned that questionParams set in infoCourse.json will "pollute" the params of all questions. That is, every variant generated in the course will include the course questionParams in its stored params. For example, in the example below where course instructor names are set in the infoCourseInstance.json question params, I think that every variant for every question in that course instance would store the instructor names?

I'm not really worried about the storage, it's more that it will be messy when we display the params for variants and for most questions there are these spurious entries.

A couple of ideas off the top of my head to address this:

  1. Use questionOptions rather than questionParams and merge into data["options"]. This is not stored on a per-variant basis. If a question wants to store the data, they can copy it from options into params.

  2. Require questions to opt-in to particular question params by listing them in their info.json file. For example, a question would need to declare "useQuestionParams": ["names"] in its info.json file to have the names question param copied into its local params.

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, I understand your concern. It's ultimately your team's call, both solutions sound easy enough for me to implement. Here are my thoughts:

  1. This is nice for more clearly differentiating "params" (which currently only come from server.py, right?) from "options" (which are passed in from external sources). I think most use of "options" will be to set bounds/configurations on the variants that are generated (at least this is true in my head), and those don't really need to be visible. As you say, we can copy options to params if we want them visible/stored.
  2. This is also nice for documenting the ways a question can be configured.

Copy link
Member

Choose a reason for hiding this comment

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

@nwalters512 thoughts about this?

Copy link
Contributor

Choose a reason for hiding this comment

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

@nealterrell Matt and just chatted about this for an hour or so. We've agreed that something more like option 2 is the way we want to go, i.e. use params that are persisted as part of a variant (not options) and require questions to declare somehow that they will or can use a certain parameter. We'd like to spend a little bit longer talking about this, as the design will have implications for other things that are part of our longer-term vision for PL, and we want to make sure that we're at least foreseeing potential problems now. Sorry the complexity is snowballing here, but we want to be really deliberate with how we're designing this.

Matt is heading off to Learning@Scale for a few days so we probably won't be able to talk about this more til next week. In the meantime, here are some of the things we're thinking about:

  • Someday, we'll allow folks to share questions between courses or institutions. How can we make sure we're guiding people towards writing sharable questions in the context of params, which may not be present in a course that a question is shared with? We're thinking about some way to declare defaults in info.json, and possibly making that mandatory.
    • The alternative is of course .get('...', DEFAULT) in Python, but if your course or course instance already has that value in place, it would be easy to forget that you should specify a default value as you'd never hit the case of a missing param during question development.
  • In the same vein as defaults, having declarative information about params in info.json would allow us to do things like make an editor or preview UI to show which params will take effect, or to layer on some kind of schema checking, and so on. We probably want something more than just a list of param names, but how much more? Maybe {"foo": {"default: 5}}? Or [{"name": "foo", "default": 5}]? Maybe we just use JSON Schema directly?
  • What do we actually want to call these things? Calling them just "params" inside a question might be a bit confusing, as they're different from the params that a question would write into params during generate. Maybe we call them "inherited params" or "external params"? @mwest1066 might be able to expand on this thought a bit.

Feel free to chime in with your thoughts about any/all of the above. I'll keep you posted with what Matt and I chat about next week.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To chime in briefly about this, having the "params" field in the data dict copied into could cause confusion in designing the backend for a question. The easiest thing (IMO) would be to have a separate entry in the data dictionary (questionParams, for example) that only holds options taken from the info.json (similar to option 2). That way, we can add questionParams to the type signatures for the data dict, and it makes it easier to declare a custom dictionary for just those parameters. This seems like a really cool feature, and something that is a good stepping stone to question sharing across courses.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I agree with @echuber2, the parameters can come from the course, instance, or assignment level, so externalParams seems more natural. @nwalters512 what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a big fan of externalParams!

Copy link
Collaborator

Choose a reason for hiding this comment

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

With respect to declaring that information in the info.json, it seems like having a dictionary of the form {"foo": {"default": 5}} as Nathan mentioned would be best, since dict keys have to be distinct (as opposed to the second list suggestion). So the entry externalParams in the info.json for a question would hold that dict, and then the course + assignment + whatever could override it as needed (with an error being thrown if a parameter is injected that isn't on the question's approved list). Handling this with the json as opposed to just in the Python backend, the error message will be thrown as soon as the assignment configuration including the question is created, as opposed to when the question is first opened.

Also I know this has been sitting around for a while, but @nealterrell let us know if you'd like any help implementing this, since our team has grown enough that we can offer some assistance if needed, and this would be a useful feature for us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eliotwrobson I am able to work on the trivial stuff like renaming the JSON fields, but the plans for parameter schemas and validation probably exceeds my bandwidth at the moment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Understood, I'll get someone on that piece of this as soon as I can. The parameter validation shouldn't be too difficult, since you already have the parameter injection set up. Will follow up later if we run into issues.

nealterrell and others added 2 commits May 27, 2022 16:15
Co-authored-by: Matthew West <mwest@illinois.edu>
Co-authored-by: Matthew West <mwest@illinois.edu>
@nwalters512
Copy link
Contributor

I had a great conversation with some Berkeley folks that's really relevant to the idea of external question params; sharing more details here:

The specific problem they're trying to solve is that they want to be able to essentially force a question into a particular state for QAing. There's no good way to do that now short of clicking "New variant" a bunch of times and praying that you get the variant you're looking for.

I proposed external question params as a possible solution to this. Since we're leaning in the direction of having questions declare their parameters in question.json, we could automatically generate a nice little UI for the instructor question preview where folks could pick specific params for their question. With a small adjustment to question code to be aware of these params, they could then pick specific values to use for this particular variant.

While I don't think such a UI should be a part of this PR, we should keep that in mind as a use case; it should then be pretty trivial to build this additional functionality on top of it.

@mfbutner
Copy link
Contributor

@mwest1066 Pointed me to this PR because I had an issue that was adjacent to the one that you discussed. My problem is that I have some questions where there is some "global constant" information that needs to be shared across multiple locations (serve.py, question.html, and the external grader) in the question. The most common piece of information is what files the students should be submitting. While I know I could copy this information around, it would be ideal to have it only in a single location so as to make it easier to change.

The only change that would be needed to support this based on what you are already doing is to extend it down to the question level as well.

@mwest1066
Copy link
Member

mwest1066 commented Apr 29, 2024

As @eliotwrobson pointed out, with this feature it would also be very helpful to implement #9781 to allow multiple assessment questions with the same QID (but different parameters).

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.

7 participants