Code

Opened 4 years ago

Closed 15 months 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)

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

Download all attachments as: .zip

Change History (17)

Changed 4 years ago by Beuc

Use yaml faster C implementation when available

Changed 4 years ago by Beuc

same, as patch

comment:1 Changed 4 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 4 years ago by ubernostrum

  • milestone 1.2 deleted

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

comment:3 Changed 4 years ago by Beuc

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

comment:4 Changed 3 years ago by lukeplant

  • Type set to Cleanup/optimization

comment:5 Changed 3 years ago by lukeplant

  • Severity set to Normal

comment:6 Changed 3 years ago by patchhammer

  • Easy pickings unset
  • Patch needs improvement set

pyyaml.patch fails to apply cleanly on to trunk

comment:7 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:8 Changed 16 months ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from new to closed

In a843539af2f557e9bdc71b9b5ef66eabe0e39e3c:

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

Thanks Beuc for the report and the initial patch.

comment:9 Changed 16 months ago by akaariai

  • Resolution fixed deleted
  • Status changed from closed to 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 16 months ago by aaugustin

  • Has patch unset
  • Patch needs improvement unset
  • Severity changed from Normal to Release blocker

comment:11 Changed 16 months ago by claudep

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 16 months ago by akaariai

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

comment:13 Changed 16 months ago by aaugustin

  • Severity changed from Release blocker to Normal

Oops, sorry.

comment:14 Changed 16 months ago by aaugustin

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 15 months ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 2e55cf580e48b02165b7aafb0d9368c714742137:

Adapted test assertion against yaml dump

Fixes #12914 (again).

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.