extmod/modujson: Allow json.loads() to preserve dict order. by andrewleech · Pull Request #6135

extmod/modujson: Allow json.loads() to preserve dict order. by andrewleech · Pull Request #6135 · micropython/micropython

When MICROPY_PY_UJSON_ORDERED_DICT is enabled.

Migrating away from using my previous #5323 PR to make all dict's ordered has unearthed a few other places we were relying on such functionality.

		break;
		case '{':
		next = mp_obj_new_dict(0);
		#if MICROPY_PY_UJSON_ORDERED_DICT

Choose a reason for hiding this comment

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

Is this a new option? Is it set anywhere?

Choose a reason for hiding this comment

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

It is a new option, and no it's not set by default anywhere. The expectation is it'll be set in make option or board profile if someone wants it... or if it's a desieable enough feature it could be made the default

Choose a reason for hiding this comment

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

Hmm, doesn't that make it almost impossible to discover? Someone has to go look at the code to see "hey, look at that! I can enable MICROPY_PY_UJSON_ORDERED_DICT!". Just wondering...

Choose a reason for hiding this comment

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

Yeah it's certainly not ideal... At the very least I should enable it on the coverage build and add a unit test - this initial PR was thrown up very quickly in the middle of a project rebase/test/release cycle on my end and I didn't want it to be forgotten.

Choose a reason for hiding this comment

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

Just put in in mpconfig.h, like all other options.

Arguably, this could be good to have enabled by MICROPY_CPYTHON_COMPAT rather than a new define as cpython does already load json's in order thanks to their ordered by default dict's.

As there are performance differences between regular and ordered dicts still in micropython I'm happy to have it as a separate feature enable flag for now.

I've now added the flag in mpconfig.h and added/updated unit test in coverage build.

		if isinstance(o, dict):
		print("sorted dict", sorted(o.items()))
		if is_coverage: # Has ordered json loads enabled
		print("dict", o.items())

Choose a reason for hiding this comment

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

Just to check, this assumes the CPython used to run the test will also preserve dict order. Do all CPython versions do this, and if not: from which version on? I.e. we whould be sure this always works.

Choose a reason for hiding this comment

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

Ah yes, that is the assumption yes. This has been the case since Python 3.6, though this aspect was written in as official in 3.7
https://docs.python.org/3.6/whatsnew/3.6.html#whatsnew36-compactdict

Arguably we should look at bringing in this same dict implementation as an option in micropython, it was shown to be significantly more efficient than the previous cpython dict and would likely be better than our current ordered dict implementation at least (certainly for very large dicts)

Choose a reason for hiding this comment

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

... and yes this is why the new unit test is failing, cpython 3.5 is used on travis :-(

Choose a reason for hiding this comment

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

Perhaps rather than making this a #define feature I should have just added the object_pairs_hook argument like in cpython:
https://docs.python.org/3.4/library/json.html#json.load

It would be more work to add this level of generic support rather than just added an ordered flag however

Choose a reason for hiding this comment

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

It would be more work to add this level of generic support rather than just added an ordered flag however

Would also be larger in code size.

Perhaps change the check so that when coverage is enabled, it just checks whether the returned dict is the same as sorted(dict).

Choose a reason for hiding this comment

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

Checking loaded dict == sorted dict results the same as the comparison I've already got in place - it works on micropython with my patch and cpython 3.6+ but does not work on cpython3.5. I can put an extra try block in there to loads with object_pairs_hook=OrderedDict for cpython though.

Choose a reason for hiding this comment

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

Ah but the idea is that CPython never runs any code for which it's behavior changes between versions, i.e. code guarded by is_coverage only gets executed by MicroPython, not CPython, and the else clause is where the expected result gets printed. Pseudocode:

if is_coverage:
  print(is_dict_with_same_content_and_sorted(a,b))
else:
  print(True)

Choose a reason for hiding this comment

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

Ah I didn't realise that's how it would work, that's brilliant, thanks. I've updated as such.

		case '{':
		next = mp_obj_new_dict(0);
		#if MICROPY_PY_UJSON_ORDERED_DICT
		// keep the locals ordered

Choose a reason for hiding this comment

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

Same comment as for other commit (plus this isn't about locals here)

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.38%. Comparing base (27b7bf3) to head (caaa296).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #6135   +/-   ##
=======================================
  Coverage   98.38%   98.38%           
=======================================
  Files         171      171           
  Lines       22297    22299    +2     
=======================================
+ Hits        21936    21938    +2     
  Misses        361      361

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

tannewt pushed a commit to tannewt/circuitpython that referenced this pull request

Mar 15, 2022

When MICROPY_PY_JSON_ORDERED_DICT is enabled.

Signed-off-by: Andrew Leech <andrew.leech@planetinnovation.com.au>