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

Defer translated string evaluation on validators. #5452

Merged

Conversation

doctoryes
Copy link
Contributor

@doctoryes doctoryes commented Sep 25, 2017

Description

This PR originated with the changes in this PR: #3438 . However, that PR stalled and was closed. This PR proposes the same change again - defer the error message string evaluation until the messages are actually used.

As stated in the original PR, our project uses a forked version of DRF only because of the issue described in #3354 . Our fork contains only one custom change - the one in this commit: edx-unsupported@3c72cb5 Our project (Open edX) performs a non-standard startup which causes DRF to be imported before the Django translation infrastructure is initialized.

An initial attempt to fix this issue was made in this PR: #3407 - but was replaced by the more recent PR: #3438

@carltongibson @xordoquy FYI
@bmedx @macdiesel FYI

Closes #3354

@doctoryes doctoryes force-pushed the jeskew/defer_string_evaluation branch from 0797b26 to 24391db Compare Sep 25, 2017
@xordoquy
Copy link
Collaborator

xordoquy commented Sep 25, 2017

Seems this went under my radar again.
Thanks for bringing it back !
PR needs to review but looks good at first sight.

@carltongibson carltongibson added this to the 3.7.0 Release milestone Sep 26, 2017
Copy link
Collaborator

@carltongibson carltongibson left a comment

OK. Lets have this. What a nice one to clear up!

I added a docstring to CustomValidatorMessage just so we can remember what this is all for in two years time.

@doctoryes @xordoquy Thanks for the work on this! Good one. 👍

@carltongibson carltongibson changed the title Customize validators to defer error string evaluation. Defer translated string evaluation on validators. Sep 26, 2017
carltongibson added a commit that referenced this issue Sep 26, 2017
@carltongibson carltongibson merged commit 607e4ed into encode:master Sep 26, 2017
1 check passed
carltongibson added a commit that referenced this issue Sep 27, 2017
carltongibson added a commit that referenced this issue Sep 28, 2017
carltongibson added a commit that referenced this issue Oct 5, 2017
carltongibson added a commit that referenced this issue Oct 5, 2017
carltongibson pushed a commit that referenced this issue Oct 6, 2017
* Set version number for 3.7.0 release

* Rename release notes section

Moved issue links to top for easier access.
(Can move back later)

* Add release note for #5273

* Add release note for #5440

* Add release note for #5265

Strict JSON handling

* Add release note for #5250

* Add release notes for #5170

* Add release notes for #5443

* Add release notes for #5448

* Add release notes for #5452

* Add release not for #5342

* Add release notes for 5454

* Add release notes for #5058 & #5457

Remove Django 1.8 & 1.9 from README and setup.py

* Release notes for merged 3.6.5 milestone tickets

Tickets migrated to 3.7.0 milestone.

* Add release notes for #5469

* Add release notes from AM 2ndOct

* Add final changes to the release notes.

* Add date and milestone link

Move issue links back to bottom.

* Update translations from transifex

* Begin releae anouncement

* Add release note for #5482

* 3.7 release announcement & related docs.
@mehdipourfar
Copy link

mehdipourfar commented Mar 27, 2019

This commit makes serialization process 3 times slower. In my project, I inspected that serializing about 160 objects takes around 1500 milliseconds. After profiling, I've found out that the most time consuming tasks are related to using of Django's lazy function:

ncalls tottime percall cumtime percall filename:lineno(function)
9126 0.428 0.000 0.865 0.000 functional.py:82(prepare_class)
847785 0.343 0.000 0.343 0.000 {built-in method builtins.hasattr}
2724/2562 0.198 0.000 0.233 0.000 fields.py:627(deepcopy)
267546 0.065 0.000 0.065 0.000 {built-in method builtins.setattr}
4386 0.057 0.000 0.080 0.000 {built-in method builtins.build_class}
8244 0.056 0.000 0.071 0.000 fields.py:320(init)
232458 0.048 0.000 0.048 0.000 functional.py:102(promise)
4431 0.039 0.000 0.273 0.000 field_mapping.py:66(get_field_kwargs)
158386 0.039 0.000 0.045 0.000 {built-in method builtins.getattr}
795 0.031 0.000 1.729 0.002 serializers.py:989(get_fields)
4386 0.024 0.000 0.024 0.000 functional.py:57(proxy)
.
.
.

After removing the usage of lazy function in restframework source code, my serialization process finishes after around 500 miliseconds.

@rpkilby
Copy link
Member

rpkilby commented Mar 27, 2019

Hm. I think this can be resolved by relying on the ValidationError to format the message, instead of us. Basically, we need to change our error messages to use %(limit_value)s and let the ValidationError format the message, instead of providing a lazily formatted {max_value}. @mehdipourfar - do you think you'd be up for creating a PR?

Although, I guess this would create a compatibility issue w/ users who already override their custom messages with their own format strings. Maybe we could detect this? idk if there's a reliable way of doing so.

Then again, detecting this would defeat the purpose of lazy evaluation.

@mehdipourfar
Copy link

mehdipourfar commented Mar 28, 2019

Hi @rpkilby
I have created a PR:
remove usage of django's lazy function for error messages

@rpkilby
Copy link
Member

rpkilby commented Mar 29, 2019

Thanks.

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.

5 participants