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

Define JSON enconding to UTF-8 #90

Merged
merged 1 commit into from
Sep 6, 2023

Conversation

by3nrique
Copy link
Contributor

@by3nrique by3nrique commented Sep 5, 2023

Pull Request Summary:

The current implementation of response.json() from the requests module involves time-consuming charset detection, particularly problematic when handling a large number of requests. (For more details, see this Stack Overflow discussion: Stack Overflow - Performance Issues When Formatting Using JSON).

This pull request introduces an enhancement to the dicomweb-client module, specifying the character set and bypass charset detection. This improvement can significantly boost performance. This is compatibledwith the requests module (https://docs.python-requests.org/en/latest/user/quickstart/#response-content)

In the specific case of DICOMWEB, the presence of the UTF-8 charset in every response is guaranteed, as dictated by the DICOM standard. More information about this requirement can be found in the DICOM standard documentation: DICOM Standard - Section F.2.

Changes:

  • Enhanced the dicomweb-client module, specifying the character set.
  • Improved performance when dealing with a high volume of requests.
  • Assured UTF-8 charset in DICOMWEB responses as per the DICOM standard.

Testing:

All tests have passed successfully.

Notes:

Thank you for your attention to this matter.

@pieper
Copy link
Member

pieper commented Sep 5, 2023

The change sounds fine to me. Not sure why the CI is failing. Any comments from @hackermd @cgorman or others?

@cgorman
Copy link
Collaborator

cgorman commented Sep 5, 2023

@pieper Ah yes numpy deprecated a method name so mypy is failing. I actually opened a PR to fix it #86 back in July but wasn't able to merge due to review requirements and then I forgot about it. Would you be able to approve it and see if we can get it merged? That should let us merge this as well.

@pieper
Copy link
Member

pieper commented Sep 5, 2023

Thanks @cgorman , I merged it.

@by3nrique would you be able to rebase to the current main so that the CI tests will run again?

@by3nrique
Copy link
Contributor Author

Thanks @pieper. I have rebased to the current main, then there should be no problem.

Copy link
Collaborator

@cgorman cgorman left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@cgorman cgorman merged commit 39dc8a7 into ImagingDataCommons:master Sep 6, 2023
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.

3 participants