allow sorting keys on to_json and to_python by passing in sort_keys

[aezomz]

Hello Pydantic Team! This is my first time contributing to a Rust and Pyo3 related repo.
I am also new in Rust.
Do you think this PR will make sense? Since I have been trying to do model_dump_json with sort keys too.
If so...
Should I also raise the error early instead of doing unwrap_or_default() in this PR?
I think we can remove to_python implementation, looks like the default sorting for dictionary is always sorted.

Codspeed benchmark actually slowed down for those two functions. Let me know how I can do better.

Based on the feedback from adriangb, its better to add in the support for recursive sort.
However, I do not sort dictionary in an array, as it add more complexity and am not very confident to add everything in one PR.

Will sort from:
        {
            'field_123': b'test_123',
            'field_b': 12,
            'field_a': b'test',
            'field_c': {'mango': 2, 'banana': 3, 'apple': 1},
            'field_d': [{'d': 3, 'b': 2, 'a': 1}, 2, 3],
        }
To:
s.to_python(m, exclude_none=True, sort_keys='recursive', mode='json')
        {
            'field_123': 'test_123',
            'field_a': 'test',
            'field_b': 12,
            'field_c': {'apple': 1, 'banana': 3, 'mango': 2},
            'field_d': [{'d': 3, 'b': 2, 'a': 1}, 2, 3],
        }

Thanks!

Change Summary

allow sorting keys on to_json and to_python by passing in sort_keys

Related issue number

should fix pydantic/pydantic#7424
Might need to create another MR on Python repo though, need to check.

Checklist

• Unit tests for the changes exist
• Documentation reflects the changes where applicable
• Pydantic tests pass with this pydantic-core (except for expected changes)
• My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @davidhewitt

[codecov]

Codecov Report

Attention: Patch coverage is 77.31481% with 49 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/serializers/fields.rs	83.03%	28 Missing ⚠️
src/serializers/extra.rs	37.50%	20 Missing ⚠️
python/pydantic_core/core_schema.py	50.00%	0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[codspeed-hq]

CodSpeed Performance Report

Merging #1637 will not alter performance

_{Comparing aezomz:allow_model_dump_sort_keys (95f9329) with main (f977516)}

Summary

✅ 157 untouched benchmarks

[aezomz]

please review

[adriangb]

Main issue is perf regression

[adriangb]

@davidhewitt this is getting gross 😢

[adriangb]

My intuition is that this extra Vec being created is the cause for the slowdown in benchmarks.

[aezomz]

I refactored the function out, so we can re-use this. and avoid creating new variable when sort keys is false

[adriangb]

Why did these have to change? Is this to make the tests change? I would prefer a new self-contained test just for this bit.

[aezomz]

seperated this.

[aezomz]

I separated the test out, I also refactor the functions so we can reuse when sort_keys=true.
To keep the original perf benchmark, I have done a simple bool check on sort_keys before using expensive function like sorting.

Let me know what else I need to improve. Thanks

[aezomz]

please review, not sure how I can take it from here

[adriangb]

Hmm I see now that this is not recursive (it only applies to the top level keys). Would it be hard to make it recursive? I fear that if we implement the non-recursive version someone is going to come along and want the recursive version... if so we can make it a Literal['recursive', 'top-level', 'unsorted'] or something like that.

[adriangb]

Since you're already collecting into a Vec here I think you should do something like this:

Suggested change

	let mut items = main_iter.collect::<PyResult<Vec<_>>>()?;
	items.sort_by_cached_key(|(key, _)| key_str(key).unwrap_or_default().to_string());
	let mut items = main_iter.map(|k,v| (key_str(key), v)).collect::<PyResult<Vec<_>>>()?;

Might need to collapse two levels of Result or something like that.

Then you'll have to convert back to the original type. I think this will be the fastest approach.

[aezomz]

Sorry, I kept the original implementation. Didn't quite get your advice, still quite new to Rust 😓
But added recursive mode.

[adriangb]

The first thing process_field_entry_python does is call key_str:

pydantic-core/src/serializers/fields.rs

Line 234 in 95f9329

let key_str = key_str(key)?;

So I think you can just convert to it from the get go

[aezomz]

Hmm I see now that this is not recursive (it only applies to the top level keys). Would it be hard to make it recursive? I fear that if we implement the non-recursive version someone is going to come along and want the recursive version... if so we can make it a Literal['recursive', 'top-level', 'unsorted'] or something like that.

Added different sort mode as above, updated the PR description.

[aezomz]

please review 👍