Opened 14 years ago

Closed 11 years ago

#12914 closed Cleanup/optimization (fixed)

Use yaml faster C implementation when available

Reported by: Beuc Owned by: nobody
Component: Core (Serialization) Version: dev
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)

pyyaml.py-svn-modified (2.2 KB ) - added by Beuc 14 years ago.
Use yaml faster C implementation when available
pyyaml.patch (971 bytes ) - added by Beuc 14 years ago.
same, as patch

Download all attachments as: .zip

Change History (17)

by Beuc, 14 years ago

Attachment: pyyaml.py-svn-modified added

Use yaml faster C implementation when available

by Beuc, 14 years ago

Attachment: pyyaml.patch added

same, as patch

comment:1 by Russell Keith-Magee, 14 years ago

Triage Stage: UnreviewedAccepted

comment:2 by James Bennett, 14 years ago

milestone: 1.2

1.2 is feature-frozen, moving this feature request off the milestone.

comment:3 by Beuc, 14 years ago

This is more a bug-fix than a feature IMHO.
I'd recommend including it in 1.2.

comment:4 by Luke Plant, 13 years ago

Type: Cleanup/optimization

comment:5 by Luke Plant, 13 years ago

Severity: Normal

comment:6 by patchhammer, 13 years ago

Easy pickings: unset
Patch needs improvement: set

pyyaml.patch fails to apply cleanly on to trunk

comment:7 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:8 by Claude Paroz <claude@…>, 11 years ago

Resolution: fixed
Status: newclosed

In a843539af2f557e9bdc71b9b5ef66eabe0e39e3c:

Fixed #12914 -- Use yaml faster C implementation when available

Thanks Beuc for the report and the initial patch.

comment:9 by Anssi Kääriäinen, 11 years ago

Resolution: fixed
Status: closednew

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 by Aymeric Augustin, 11 years ago

Has patch: unset
Patch needs improvement: unset
Severity: NormalRelease blocker

comment:11 by Claude Paroz, 11 years ago

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 by Anssi Kääriäinen, 11 years ago

If it is just a representation error then the above fix seems fine.

comment:13 by Aymeric Augustin, 11 years ago

Severity: Release blockerNormal

Oops, sorry.

comment:14 by Aymeric Augustin, 11 years ago

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 by Claude Paroz <claude@…>, 11 years ago

Resolution: fixed
Status: newclosed

In 2e55cf580e48b02165b7aafb0d9368c714742137:

Adapted test assertion against yaml dump

Fixes #12914 (again).

Note: See TracTickets for help on using tickets.
Back to Top