Opened 10 years ago
Closed 7 years ago
#12914 closed Cleanup/optimization (fixed)
Use yaml faster C implementation when available
Reported by: | Beuc | Owned by: | nobody |
---|---|---|---|
Component: | Core (Serialization) | Version: | master |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
pyyaml does not use the C implementation automatically.
Instead, it uses the Python implementation which is at least twice as slow.
This is one of the first thing that the documentation covers:
http://pyyaml.org/wiki/PyYAMLDocumentation#Installation
The attached patch uses the C implementation when available.
It would be nice to add a warning when the Python implementation is used, so that users are encouraged to install libyaml, but I'm not sure how to do it properly.
Attachments (2)
Change History (17)
Changed 10 years ago by
Attachment: | pyyaml.py-svn-modified added |
---|
comment:1 Changed 10 years ago by
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 Changed 10 years ago by
milestone: | 1.2 |
---|
1.2 is feature-frozen, moving this feature request off the milestone.
comment:3 Changed 10 years ago by
This is more a bug-fix than a feature IMHO.
I'd recommend including it in 1.2.
comment:4 Changed 9 years ago by
Type: | → Cleanup/optimization |
---|
comment:5 Changed 9 years ago by
Severity: | → Normal |
---|
comment:6 Changed 9 years ago by
Easy pickings: | unset |
---|---|
Patch needs improvement: | set |
pyyaml.patch fails to apply cleanly on to trunk
comment:8 Changed 7 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:9 Changed 7 years ago by
Resolution: | fixed |
---|---|
Status: | closed → new |
The commit is causing some failures in timezone tests (there might be other failures, too, haven't ran the full suite):
(py3_venv)akaariai@akaariai-UX31E:~/Programming/django/tests$ ./runtests.py --settings=test_sqlite timezones Creating test database for alias 'default'... Creating test database for alias 'other'... .......sssssss....s..............s......s...............FFFFFF................. ====================================================================== FAIL: test_aware_datetime_in_local_timezone (modeltests.timezones.tests.SerializationTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/akaariai/Programming/django/tests/modeltests/timezones/tests.py", line 625, in test_aware_datetime_in_local_timezone self.assert_yaml_contains_datetime(data, "2011-09-01 13:20:30+03:00") File "/home/akaariai/Programming/django/tests/modeltests/timezones/tests.py", line 507, in assert_yaml_contains_datetime self.assertIn("- fields: {dt: !!timestamp '%s'}" % dt, yaml) AssertionError: "- fields: {dt: !!timestamp '2011-09-01 13:20:30+03:00'}" not found in "- fields: {dt: ! '2011-09-01 13:20:30+03:00'}\n model: timezones.event\n pk: null\n" <SNIP similar errors...> ---------------------------------------------------------------------- Ran 79 tests in 0.332s FAILED (failures=6, skipped=10)
The failures happen only if pyyaml is installed, and only on Python 3. Here is freeze of the py3_venv:
Markdown==2.2.1 PyYAML==3.10 cx-Oracle==5.1.2 distribute==0.6.30 djangobench==0.9 docutils==0.10 flake8==1.7.0 ipdb==0.7 ipython==0.13.1 psycopg2==2.4.6 pytz==2012j textile==2.1.5 wsgiref==0.1.2
comment:10 Changed 7 years ago by
Has patch: | unset |
---|---|
Patch needs improvement: | unset |
Severity: | Normal → Release blocker |
comment:11 Changed 7 years ago by
Aymeric, this was not committed in 1.5. Did you still want to set this as a blocker?
I can also reproduce it with Python 2. Apparently, this is only a difference in representation, and loading both formats seems to give back the appropriate datetime object.
So this might be simply solved by changing the test assertion
- self.assertIn("- fields: {dt: !!timestamp '%s'}" % dt, yaml) + # Depending on the yaml dumper, '!timestamp' might be absent + self.assertRegexpMatches(yaml, + r"- fields: {dt: !(!timestamp)? '%s'}" % re.escape(dt))
comment:12 Changed 7 years ago by
If it is just a representation error then the above fix seems fine.
comment:14 Changed 7 years ago by
PyYAML isn't very good with datetimes in general. I don't like changing tests because we can't get the output we want, but in this case, we don't have much choice.
PyYAML's documentation helpfully states that "there are some subtle (but not really significant) differences between pure Python and LibYAML based parsers and emitters" which may include the difference we're seeing here. I'm not sure it's worth reporting this as a bug.
Apparently PyYAML moved to bitbucket two weeks ago, after 18 months without any activity.
comment:15 Changed 7 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
Use yaml faster C implementation when available