Skip to content

Port to orjson from ujson #8584

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

Closed
wants to merge 1 commit into from
Closed

Conversation

aadya940
Copy link

As specified in #8540 , ujson is in maintanance mode.

@@ -318,7 +318,7 @@ def _save_data_to_local_file(train_data: list[dict[str, Any]], data_format: Trai
elif data_format == TrainDataFormat.COMPLETION:
_validate_completion_data(item)

f.write(ujson.dumps(item) + "\n")
f.write(orjson.dumps(item).decode() + "\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend changing the open(file_path, "w"), above, to "wb" mode to eliminate the decodes happening here.

The end result will be:

Suggested change
f.write(orjson.dumps(item).decode() + "\n")
f.write(orjson.dumps(item) + b"\n")

Copy link
Collaborator

Choose a reason for hiding this comment

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

decode() is beneficial here - one nice benefit of saving state as json is the file is human readable, so that users can copy/paste the demos/instructions to other parts of the pipeline. I have seen many users only use DSPy as the prompt optimization tool, and export the optimized prompt (instruction + demos).

Copy link
Contributor

Choose a reason for hiding this comment

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

.decode() is not doing the beneficial work that you think it's doing.

It's creating this pipeline:

with open(file_path, "w") as f:

Opening the file in text mode means that Python must internally encode the file when it actually writes the file. Python does NOT use UTF-8 for this! It defaults to the system-specific encoding. On Windows in the U.S., for example, this is usually something like ISO-8859-1.

orjson.dumps(item)

This produces a bytes object -- perfect for writing to disk!

orjson.dumps(item).decode()

This decodes the bytes to UTF-8, and is a performance problem because now Python must re-encode the content before it can be written to disk. What a waste.

orjson.dumps(item).decode() + "\n"

Another performance problem -- the entire JSON string must now be copied in memory just so a newline can be added to the end in memory.

f.write(orjson.dumps(item).decode() + "\n")

Here's the full pipeline in this one line of code:

  • orjson produces a UTF-8-encoded bytes instance.
  • The bytes are decoded using UTF-8.
  • A new string is constructed in memory, with a newline added to the end.
  • Python re-encodes the string to the default system character set, which varies from one system to the next.
  • Python writes the bytes instance it just created to disk.

The .decode() is not beneficial. It's a waste of cycles.

The most performant option that I'm aware of, which eliminates the decode-with-UTF-8/re-encode-with-??? roundtrip, and eliminates the construct-a-new-string-in-memory is:

with open(file_path, "wb") as f:
    f.write(orjson.dumps(item))
    f.write(b"\n")

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes I am aware of the performance issue. I was worrying about the displayed text when opening the json file is not readable, but it's not really the case since most text editors decode by utf-8 by default.

Comment on lines 61 to +63
with open(file_path, "w") as f:
for item in data:
f.write(ujson.dumps(item) + "\n")
f.write(orjson.dumps(item).decode() + "\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, I recommend eliminating the calls to .decode():

    with open(file_path, "wb") as f:
        for item in data:
            f.write(orjson.dumps(item) + b"\n")

This suggestion applies to the other change in this file.

Comment on lines 251 to +252
with open(path, encoding="utf-8") as f:
state = ujson.loads(f.read())
state = orjson.loads(f.read().encode('utf-8'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Last place I'll leave feedback, but in general the suggestion is "don't decode content only to re-encode it immediately:

            with open(path, "rb") as f:
                state = orjson.loads(f.read())

Also, these are pathlib objects so this is more ideal:

            state = orjson.loads(path.read_bytes())

@okhat
Copy link
Collaborator

okhat commented Aug 10, 2025

Thanks so much @aadya940 and thanks @kurtmckee for the comments!

@aadya940 Mind addressing the failures (ruff mostly? or maybe some tests?) and checking out the comments (I didn't dive into them)

0xEval pushed a commit to 0xEval/dspy-orjson-mig that referenced this pull request Aug 13, 2025
@chenmoneygithub
Copy link
Collaborator

@aadya940 Thanks for the PR! the most concerning part is if switching to orjson can still load a saved program before this PR. I am running some testing.

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.

4 participants