added check_dict method #9436

harivamsi9 · 2024-02-18T21:04:39Z

Can you please review this PR and let me know the feedback?

This PR will resolve #8831

eliotwrobson

Early comments. Possibly over-engineering things a little bit but I think this function should be fairly robust to fill a couple of different use cases.

eliotwrobson · 2024-02-18T21:45:15Z

graders/python/python_autograder/code_feedback.py

+                return False
+
+        if data is None:
+            return bad("'%s' is None or not defined" % name)


Suggested change

return bad("'%s' is None or not defined" % name)

return bad(f"{name} is None or not defined")

Throughout the new changes, should use f-strings instead of the %.

eliotwrobson · 2024-02-18T21:46:14Z

graders/python/python_autograder/code_feedback.py

+            return bad("'%s' is None or not defined" % name)
+
+        if not isinstance(data, dict):
+            return bad("'%s' is not a list" % name)


Suggested change

return bad("'%s' is not a list" % name)

return bad(f"{name} is not a dict")

eliotwrobson · 2024-02-18T21:47:34Z

graders/python/python_autograder/code_feedback.py

+        """
+        Feedback.check_dict(name, ref, data)
+
+        Check that a student dict has correct length with respect to a reference dict.  Can also check for a homogeneous data type for the list.


It says that only the length is checked for, but it looks like this function checks for equality between the dicts.

eliotwrobson · 2024-02-18T21:49:02Z

graders/python/python_autograder/code_feedback.py

+        if ref == data:
+            return True
+        else:
+            return bad("'%s' is incorrect" % (name))


Having more detailed feedback would be useful, since it's not necessarily clear whether the keys or keys and values are being checked for equality to the end user.

eliotwrobson · 2024-02-18T21:50:01Z

graders/python/python_autograder/code_feedback.py

+        - ``data``: Student dictionary to be checked.
+        - entry_type: If not None, checks if each value in the student dictionary has this type.
+        - ``accuracy_critical``: If true, grading will halt on failure.
+        - ``report_failure``: If true, feedback will be given on failure.


Some more parameters would be useful, like checking keys and values or just values only, and maybe even checking that the student answer contains some keys in particular instead of just equality.

I have added:

partial_keys: If not None, it takes a List of keys to check if these particular keys are present in the student's dict or not.

check_only_values: If true, grading will be done only based on checking all valeus in student's dict and reference's dict match or not.

check_only_keys: If true, grading will be done only based on checking all keys in student's dict and reference's dict match or not.

entry_type_key: If not None, requires that each key in the student's dictionary in solution be of this type.

entry_type_value: If not None, requires that each value in the student's dictionary in solution be of this type.

eliotwrobson

More comments. Some of the functionality here has some overlapping use cases that could be confusing to end users. We're coming up with the interface for this on the fly, so not a huge deal if it takes a couple rounds of review to iron things out.

eliotwrobson · 2024-02-19T17:40:00Z

docs/python-grader/index.md

@@ -164,6 +164,8 @@ The code feedback library contains built-in functions for checking correctness o
  Checks that a numpy array has the same shape and datatype as the reference solution, also checks values to see if they are close using specified `rtol` and `atol`.
 - `check_list(name, ref, data, entry_type=None)`
  Checks that a list has the same length as the reference solution. If `entry_type` is not `None`, can optionally check if each element has that type. Does _not_ check values against the reference.
+- `check_dict(name, ref, data, entry_type_key=None, entry_type_value=None)`


Function signature here is missing new keyword arguments.

eliotwrobson · 2024-02-19T17:43:09Z

graders/python/python_autograder/code_feedback.py

+            if check_only_keys:
+                for key in data.keys():
+                    if key not in ref.keys():
+                        return bad(f"{name} contains an extra key: {key}")


No error gets reported if there is a key in ref.keys() that is not in data.keys() (i.e. extra keys). This functionality is confusing since it seems like this use case should be covered by check_only_keys, but is instead by partial_keys. I think these should be consolidated, or change partial_keys into a flag that disables the length equality check.

@eliotwrobson thanks for pointing this out!

The reason why I did not add additional code for "No error gets reported if there is a key in ref.keys() that is not in data.keys() (i.e. extra keys)." functionality is because, we already perform this check as a default check in the check_dict() function Line: 347

Ref: https://github.com/PrairieLearn/PrairieLearn/pull/9436/files#r1574047293

Please let me know if any changes are still required on this.

Otherwise, I could change the line 347
if len(ref) != len(data):
to

if partial_keys != None and len(ref) != len(data):

So that, when we are only checking for partial keys, we skip the the length equality check.

I think that the second choice (adding the partial_keys check) makes things clearer.

eliotwrobson · 2024-02-19T17:45:03Z

docs/python-grader/sphinx-docs.md

@@ -62,6 +62,30 @@ Check that a student list has correct length with respect to a reference list. C

 - `report_failure`: If true, feedback will be given on failure.

+#### classmethod check_dict(name, ref, data)
+
+Checks that a student dict has all correct key-value mappings with respect to a reference dict. Can also check that a student dict has correct length of keys with respect to a reference dict's length of keys.  Can also check for a homogeneous data type for either the dict's key, dict's value, or both. Can also check if only some particular keys present in the student dict or not. Can also check student dict with respect to a reference dict only based on either keys, values, or both.


I think this is a little confusing. The check_only_keys works mostly the way I would expect, but check_only_values checks that the set of values is equal, but ignores the keys they are mapped to. I think we should get rid of the check_only_values flag but keep the check_only_values flag (defaulting to full equality if this flag is false).

I believe the current code covers this functionality. (Lines 368 to 393)

i.e.
if check_only_values flag is True, then we solely evaluate the student's submission based on the "set of values is equal, but ignores the keys they are mapped to."

and if check_only_values flag is False, then we perform the proper full equality i.e. ref == data Line 391.

Maybe I need to write the documentation more elaborately.

@eliotwrobson Is this what you are looking for? or please let me know if you need any changes here.

I think that makes sense. More detailed documentation would make the logic much easier to follow.

Awesome! I will update the documentation to keep it unambiguous

eliotwrobson · 2024-02-19T17:45:46Z

graders/python/python_autograder/code_feedback.py

+            ref == data
+        ):  # will check equality of both keys and values between reference dict and student's dict
+            return True
+        else:


Suggested change

else:

No else: needed, since the if case above returns (so can just return the result of calling bad).

eliotwrobson · 2024-02-19T17:47:02Z

graders/python/python_autograder/code_feedback.py

+
+        if len(ref) != len(data):
+            return bad(
+                f"{name} has the wrong length for keys--expected {len(ref)}, got {len(data)}"


Suggested change

f"{name} has the wrong length for keys--expected {len(ref)}, got {len(data)}"

f"{name} has the wrong number of entries, expected {len(ref)}, got {len(data)}"

eliotwrobson · 2024-02-19T17:47:18Z

graders/python/python_autograder/code_feedback.py

+            )
+
+        if entry_type_value is not None:
+            for _, value in data.items():


Suggested change

for _, value in data.items():

for value in data.values():

eliotwrobson · 2024-02-19T17:48:21Z

graders/python/python_autograder/code_feedback.py

+                    return bad(f"{name} has the wrong type for value {value}")
+
+        if entry_type_key is not None:
+            for key, _ in data.items():


Suggested change

for key, _ in data.items():

for key in data.keys():

eliotwrobson · 2024-03-19T02:52:59Z

@harivamsi9 do you think you'll be able to see this through? Sorry if the ambiguity from me is slowing things down here, if things have gotten a little confusing I'd be happy to post on slack to get some clearer direction on what use cases others may want.

harivamsi9 · 2024-03-19T03:46:08Z

Hey @eliotwrobson Sorry for the delay, I was procrastinating things! I will wrap this up by tomorrow!
Happy to chat about any additional changes required over slack.

eliotwrobson · 2024-04-11T18:42:16Z

@harivamsi9 any updates here? If you're too busy, I'm happy to take over this PR, but it looks like it's in a pretty close state to being done. We'd like to have this done before the end of the month if possible 🙏🏽

harivamsi9 · 2024-04-13T23:41:54Z

hey @eliotwrobson i will finish it off. sorry for the delay. i will do this ASAP

harivamsi9 · 2024-04-22T02:23:16Z

graders/python/python_autograder/code_feedback.py

+        if not isinstance(data, dict):
+            return bad(f"{name} is not a dict")
+
+        if len(ref) != len(data):


Line: 347
Here is where we check "No error gets reported if there is a key in ref.keys() that is not in data.keys() (i.e. extra keys)."

harivamsi9 · 2024-04-22T03:38:12Z

Hey @eliotwrobson, I believe I have covered all the review items, and also added more detailed documentation.
I do not know why the CI/native-checks (pull_request) is failing. Please let me know what can I do to rectify this.

Apart from that, I believe this PR is good to go. Could you please review this and merge if not issues found?

eliotwrobson · 2024-04-22T03:41:50Z

@harivamsi9 will take a look soon! The native checks are failing because of the formatting in the documentation. You have to run prettier over all of the markdown files you change (this can be done by the make format command in the directory https://prairielearn.readthedocs.io/en/latest/dev-guide/#coding-style).

harivamsi9 · 2024-04-22T05:03:42Z

@harivamsi9 will take a look soon! The native checks are failing because of the formatting in the documentation. You have to run prettier over all of the markdown files you change (this can be done by the make format command in the directory https://prairielearn.readthedocs.io/en/latest/dev-guide/#coding-style).

When I try to fix this, I encounter this error

I tried the npm install @prairielearn/eslint-plugin@latest --save-dev but that did not help either.

Can you please help me with this?

nwalters512 · 2024-04-22T05:48:35Z

You'll need to run make deps first. In general, you should follow our documentation for setting up your dev environment. Pick the appropriate one depending on if you're running natively or in Docker:

harivamsi9 · 2024-04-22T17:02:44Z

You'll need to run make deps first. In general, you should follow our documentation for setting up your dev environment. Pick the appropriate one depending on if you're running natively or in Docker:

https://prairielearn.readthedocs.io/en/latest/installingLocal/

https://prairielearn.readthedocs.io/en/latest/installingNative/

While replicating the steps given for docker, when I enter make deps, I encounter the following error.

I tried to resolve this, yet did not find any solution. So I went ahead with the make dev command which gave me this error

Please let me know if there is something I am doing wrong here.

Meanwhile, I am retrying with https://prairielearn.readthedocs.io/en/latest/installingNative/ approach...

harivamsi9 · 2024-04-22T17:10:19Z

Also, just an another idea,
If we know that this error usually comes up when formatting in the documentation, can we perform this make format command in the CI itself? or may be using something like a git hooks perhaps. Please correct me if I am wrong here

nwalters512 · 2024-04-22T19:48:42Z

I see KeyboardInterupt in your error message, did you press Ctrl/Cmd-C while running? Did you try running make deps again? If that fails, other things won't work either.

We generally don't want CI to mutate branches, and because of how many folks tend to do PrairieLearn development (inside a Docker container), Git hooks wouldn't work well because the necessary dependencies wouldn't be installed in a place that's accessible to any such hooks.

harivamsi9 · 2024-04-23T03:09:09Z

I see KeyboardInterupt in your error message, did you press Ctrl/Cmd-C while running? Did you try running make deps again? If that fails, other things won't work either.

We generally don't want CI to mutate branches, and because of how many folks tend to do PrairieLearn development (inside a Docker container), Git hooks wouldn't work well because the necessary dependencies wouldn't be installed in a place that's accessible to any such hooks.

Ohh I see.

It is kinda strange, because I did not press cmd/ctrl+c or any other keyboard interrupts. I used both the latest PL pull and as well as the with my branch.

Update: Finally, ALL GREENS ✅ AT LAST, was able to setup everything properly when I used the 2nd approach (i.e. InstallingNative), and make the make format work. Also, the problem still persists on the docker approach (i.e. installingLocal).

I would like to point out that the "Install the prerequisites:" sections needs to be updated to include R software as well. There was a problem when running make deps which will invoke pip/pip3 install rpy2, but this throws up an error like

Upon debugging further, I found out that R was required to be installed (reference: https://stackoverflow.com/questions/17573988/r-home-error-with-rpy2). So I believe it will be helpful to add R also into the "Install the prerequisites:", so it is a bit easier for the first-timers to on-board.

That being said, this is my First proper PR to PraireLearn, from being a Teaching Assistant for CSE 8A: Intro to Programming at UCSD to using PraireLearn to code/build/administrate our exams, quizes, homeworks to identifying small gaps in the system and proposing ideas, and actually contributing to the software.

As a few next steps, I am planning on creating content for YT, Insta, Twitter(X), LinkedIn to talk about this PR, and also to help people start with OSS and also to build in public. I deeply appreciate @eliotwrobson's and @nwalters512's support throughout this, and I am interested to keep contributing to develop new features, take up bigger tasks, write high quality code, and scale PraireLearn.

nwalters512 · 2024-04-23T03:26:24Z

Huh, I really don't know what's happening when you try to execute in Docker. If you want to get that working, you could start a GitHub Discussion to look at that in more detail (PR comments aren't the best venue to debug dev setup). That error doesn't look like anything I've seen before, and if there's a possibility it could impact other people, it'd be nice to figure out what's going on.

R is a weird one, we definitely want better guidance there. Installing R is of course on option, but since it's being phased out, you can also just temporarily comment out rpy2 from the requirements file before running make deps. Sorry if you wasted time on that! You could PR some docs changes if you'd like to improve that for the next folks to use those instructions.

We're glad to have you here contributing 🙌

eliotwrobson

A bit of an incomplete review, but I wanted to give one piece of feedback early. Do you think you could write a couple of test cases for this new function? The Python autograder doesn't have a ton of dev tools set up, so it would be much easier to verify correctness of the changes with some test cases.

We're using pytest for these. It should be possible to add a new test file by looking at the pyproject.toml file in the root directory. You can add the new test file to a new test-subdirectory in the python autograder folder. I know these are somewhat vague instructions, so please follow up with me over DM on slack if you have trouble setting up this new test file.

eliotwrobson · 2024-04-23T03:26:15Z

...ted/MarkovChainGroupActivity/MarkovChains-PageRank/serverFilesQuestion/Markov-Chains-3.ipynb

It looks like a bunch of unrelated changes to the jupyter notebooks got committed? We'll need to remove before merging.

yes, these jupyter notebooks are modified with the make format cmd. I assumed that some of the previous merges might have missed this, so I committed even those.
I will remove them, and keep this PR only specific to my task.

I am familiar with using pytest. I need to progress on my other project, I will get back to working on this tomorrow, and connect with you over slack if I needed any further clarifications. Thank you 😊.

nwalters512 · 2024-05-09T17:49:18Z

@harivamsi9 Eliot will be largely unavailable this summer, so I'll be the point of contact / reviewer going forward!

added check_dict method

279a397

eliotwrobson requested changes Feb 18, 2024

View reviewed changes

harivamsi9 added 3 commits February 18, 2024 16:24

modified based on suggestions provided

cb89161

modified based on suggestions provided

8b554dc

modified based on suggestions provided

1028872

harivamsi9 requested a review from eliotwrobson February 19, 2024 02:56

eliotwrobson reviewed Feb 19, 2024

View reviewed changes

Merge branch 'master' into master

899af8f

harivamsi9 added 2 commits April 18, 2024 07:05

Merge branch 'PrairieLearn:master' into master

ac436a8

Merge branch 'PrairieLearn:master' into master

cbfc491

harivamsi9 commented Apr 22, 2024

View reviewed changes

harivamsi9 added 3 commits April 21, 2024 19:52

modified based on suggestions provided

ec42690

modified documentation to make things clear

5266743

modified documentation to make things clear

b51b9b6

harivamsi9 added 2 commits April 22, 2024 17:02

Merge branch 'master' into master

6cd7710

performed make format cmd

d70c246

eliotwrobson requested changes Apr 23, 2024

View reviewed changes

harivamsi9 added 2 commits May 2, 2024 09:01

Merge branch 'master' into master

ecfcfc8

Merge branch 'master' into master

0897e06

Merge branch 'master' into master

622bcce

	return bad("'%s' is None or not defined" % name)
	return bad(f"{name} is None or not defined")

	return bad("'%s' is not a list" % name)
	return bad(f"{name} is not a dict")

	f"{name} has the wrong length for keys--expected {len(ref)}, got {len(data)}"
	f"{name} has the wrong number of entries, expected {len(ref)}, got {len(data)}"

added check_dict method #9436

Are you sure you want to change the base?

added check_dict method #9436

Conversation

harivamsi9 commented Feb 18, 2024 • edited by eliotwrobson

eliotwrobson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliotwrobson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harivamsi9 Apr 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harivamsi9 Apr 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliotwrobson commented Mar 19, 2024

harivamsi9 commented Mar 19, 2024 • edited

eliotwrobson commented Apr 11, 2024

harivamsi9 commented Apr 13, 2024 • edited

Choose a reason for hiding this comment

harivamsi9 commented Apr 22, 2024

eliotwrobson commented Apr 22, 2024

harivamsi9 commented Apr 22, 2024 • edited

nwalters512 commented Apr 22, 2024

harivamsi9 commented Apr 22, 2024

harivamsi9 commented Apr 22, 2024

nwalters512 commented Apr 22, 2024

harivamsi9 commented Apr 23, 2024 • edited

nwalters512 commented Apr 23, 2024 • edited

eliotwrobson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harivamsi9 Apr 23, 2024 • edited

Choose a reason for hiding this comment

nwalters512 commented May 9, 2024

harivamsi9 commented Feb 18, 2024 •

edited by eliotwrobson

harivamsi9 Apr 22, 2024 •

edited

harivamsi9 Apr 22, 2024 •

edited

harivamsi9 commented Mar 19, 2024 •

edited

harivamsi9 commented Apr 13, 2024 •

edited

harivamsi9 commented Apr 22, 2024 •

edited

harivamsi9 commented Apr 23, 2024 •

edited

nwalters512 commented Apr 23, 2024 •

edited

harivamsi9 Apr 23, 2024 •

edited