Test against django 1.11(b1) / python 3.6 #271

meshy · 2017-03-02T22:52:04Z

This builds django-polymorphic against python 3.6 and django 1.11b1.

The new version of python works fine, but django 1.11 fails.

~~The test suite now runs checks, and it seems to be failing on one of them:~~
~~polymorphic.MRODerived: (models.E005) The field 'id' from parent model 'polymorphic.mrobase3' clashes with the field 'id' from parent model 'polymorphic.mrobase1'.~~
~~Once that's addressed, I suspect we're going to get a little closer to the cause of #267.~~

Ok, fixed that, now we're seeing #267.

codecov-io · 2017-03-02T22:57:53Z

Codecov Report

Merging #271 into master will increase coverage by <.01%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master     #271      +/-   ##
==========================================
+ Coverage   73.04%   73.04%   +<.01%     
==========================================
  Files          31       31              
  Lines        2478     2482       +4     
==========================================
+ Hits         1810     1813       +3     
- Misses        668      669       +1

Impacted Files	Coverage Δ
polymorphic/tests/init.py	`100% <100%> (ø)`	✅
polymorphic/tests/test_orm.py	`99.59% <66.66%> (-0.21%)`	❌

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2aff50...aefb7da. Read the comment docs.

meshy · 2017-03-02T23:24:19Z

The failing check appears to match django ticket 22442.

The affected model, MRODerived, ultimately appears to have been added in the initial commit, but I can't quite see what the intent behind it was.

meshy · 2017-03-02T23:27:52Z

It would appear that only one test references this model.

Because the test appears to relate to manager inheritance, I presume we can go with the solution proposed in the django ticket that suggests manually assigning a name to the id field of each parent class.

I'll give that a shot.

meshy · 2017-03-02T23:37:05Z

That seems to have done the trick. The tests now confirm the error outlined in #267.

This avoids the following error in django 1.11 tests: polymorphic.MRODerived: (models.E005) The field 'id' from parent model 'polymorphic.mrobase3' clashes with the field 'id' from parent model 'polymorphic.mrobase1'. Related to https://code.djangoproject.com/ticket/22442

meshy · 2017-03-03T22:47:35Z

I did a bisect on django to find the cause of the incompatibility.

So far, I have found two troublesome commits.

django/django@48826aa: This causes some expected repr strings to change in a single test. This was an easy fix.
django/django@f3b7c05: This causes Only base classes provided in Django 1.11b1 #267 and the majority of the failing tests. This one I'm still investigating.

meshy · 2017-03-03T22:52:58Z

polymorphic/tests/test_orm.py

@@ -360,7 +360,11 @@ def test_manytomany_field(self):
        # no pretty printing
        ModelShow1_plain.objects.create(field1='abc')
        ModelShow2_plain.objects.create(field1='abc', field2='def')
-        self.assertEqual(qrepr(ModelShow1_plain.objects.all()), '<QuerySet [<ModelShow1_plain: ModelShow1_plain object>, <ModelShow2_plain: ModelShow2_plain object>]>')
+        # repr classnames are not hardcoded in 1.11+


Should say "django 1.11" for clarity.

See https://code.djangoproject.com/ticket/27546 and django/django@48826aa

meshy · 2017-03-03T22:57:03Z

polymorphic/tests/test_orm.py

+        if django.VERSION >= (1, 11):
+            self.assertEqual(qrepr(ModelShow1_plain.objects.all()), '<PolymorphicQuerySet [<ModelShow1_plain: ModelShow1_plain object>, <ModelShow2_plain: ModelShow2_plain object>]>')
+        else:
+            self.assertEqual(qrepr(ModelShow1_plain.objects.all()), '<QuerySet [<ModelShow1_plain: ModelShow1_plain object>, <ModelShow2_plain: ModelShow2_plain object>]>')


I wasn't sure about repeating the whole line (I thought it wasn't very DRY), but I noticed that this approach was favoured in other similar parts of this file. If you'd prefer it changed to be more DRY, please let me know.

I think there is a nicer way to do this. The qrepr() function is already a custom invention of our own, to avoid the difference of the queryset __repr__ between Django 1.10 and earlier versions. You could upgrade the qrepr() instead to behave like Django 1.11 does, and upgrade the tests accordingly. This would remove the if/else all together.

meshy · 2017-03-03T23:40:02Z

I think this is happening because PolymorphicQuerySet.iterator() is no longer being called by _fetch_all().

It was suggested in #django-dev (on freenode) that I could try overriding _iterator_class on the queryset, but I suspect that this will be problematic as there are explicit class-checks in there, and more than one _iterator_class is in use.

timgraham · 2017-03-10T13:59:59Z

I'm not sure what should be done but I don't think the implementation of _fetch_all() should be constrained by backwards compatibility concerns.

@francoisfreitag, any advice?

francoisfreitag · 2017-03-10T20:53:21Z

From what I read in the code, I think the best way to go would be to remove the custom iterator() implementation. Instead, I suggest sub-classing ModelIterable and updating the _iterable_class attribute of the PolymorphicQuerySet to use the following PolymorphicModelIterable (untested):

class PolymorphicModelIterable(ModelIterable):
    def __iter__(self):
        base_result_objects = super(PolymorphicModelIterable, self).__iter__()
        for obj in base_result_objects:
            yield obj.get_real_instance()

I see some advantages to this approach:

The iterator() method can be removed from PolymorphicQuerySet, to use Django version that's now able to fetch objects using server side cursors (Oracle, PostgreSQL).
Polymorphic_QuerySet_objects_per_request can be removed, along with the CHUNK_SIZE constant.
self.polymorphic_disabled can be removed, since non polymorphic QuerySet can use ModelIterable directly. Otherwise, the _iterable_class can be set to PolymorphicModelIterable.

About the explicit class-check, I think Django should check for not isinstance(queryset._iterable_class, ModelIterable) instead of queryset._iterable_class is not ModelIterable. This way, PolymorphicModelIterable (and other ModelIterable subclasses) can be used in prefetch_related. @timgraham if you agree, I'll open a ticket on Django and submit a PR.

In the meantime, django-polymorphic can bypass this check overriding the Prefetch __init__ method.

I hope that's helpful. If that's unclear, I can try to submit a patch during the week-end?

meshy · 2017-03-10T22:53:53Z

That's fantastic, thank you @francoisfreitag. I think I see what you're getting at. I'll see if I can get it working with that, but I might need to take you up on the offer of a patch if I can't get it working.

The change to django seems like a good idea to me, and would be quite welcome. Would we also need to override the values() and values_list() methods to change the _iterator_class assigned there?

@vdboor, quite a significant change has been proposed here. As you appear to be the main contributor for this repo -- can we get your go-ahead on this idea?

meshy · 2017-03-10T22:56:02Z

Might this mean that we lose some of the efficiencies afforded to us by get_real_instances()?

francoisfreitag · 2017-03-12T05:22:22Z

Would we also need to override the values() and values_list() methods to change the _iterator_class assigned there?

~~If you want to remove polymorphic_disabled, yes.~~ Django values and values_list already change the _iterable_class, Would there be any reason to override them?

Might this mean that we lose some of the efficiencies afforded to us by get_real_instances()?

The first version would have lost some of the efficiencies. Would something like this be suitable?

class PolymorphicModelIterable(ModelIterable):
    def __iter__(self):
        base_result_objects = super(PolymorphicModelIterable, self).__iter__()
        real_instances = self.queryset._get_real_instances(base_result_objects)
        for obj in real_instances:
            yield obj

IIUC, the first version is more efficient if only a small portion of the results is retrieved, because the code would only fetch objects from the database on demand, versus fetching everything and keeping the results in a cache (real_instances) in the second version. However, the second version should drastically reduce the number of queries when most models are from a subclass.

vdboor · 2017-04-06T12:01:57Z

hi @meshy: Thanks for providing the pull request for 1.11 changes!
I've merged it, and made it check against the final Django 1.11 release.

I like the idea of using the native iterator functionality of Django.
This is something I really didn't get into yet.

I'd be really happy to see more people taking up maintenance work. Lots changed since I took this up in 2013 (for Django 1.5! compatibility). If you're interestted @meshy, let me know!

meshy · 2017-04-12T18:22:33Z

Hi @vdboor! Thanks for the merge :)

I'd like to have a go at getting this working on django 1.11, but I'm not sure I currently have the time to be a maintainer. That said, I'm working on a project that depends on this one, so I still plan to help out when I can :)

meshy · 2017-04-12T19:00:53Z

@francoisfreitag: my preliminary test of your second suggestion is very promising, thank you. It makes django 1.11 pass on all tests (with a little tweaking)! Unfortunately, only django 1.11 passes -- all other versions fail :D

I'll keep investigating, and will open a new PR when I have something worth looking at.

francoisfreitag · 2017-04-19T18:46:39Z

About the explicit class-check, I think Django should check for not isinstance(queryset._iterable_class, ModelIterable) instead of queryset._iterable_class is not ModelIterable. This way, PolymorphicModelIterable (and other ModelIterable subclasses) can be used in prefetch_related. @timgraham if you agree, I'll open a ticket on Django and submit a PR.

This issue has been handled in https://code.djangoproject.com/ticket/28096.

meshy force-pushed the new-test-versions branch from c90c982 to 39b7fb1 Compare March 2, 2017 23:08

meshy changed the title ~~New test versions~~ Django 1.11 / python 3.6 support Mar 2, 2017

meshy changed the title ~~Django 1.11 / python 3.6 support~~ Django 1.11(b1) / python 3.6 support Mar 2, 2017

meshy mentioned this pull request Mar 2, 2017

Test against django 1.11 (and now released python 3.6) meshy/django-conman#100

Merged

4 tasks

meshy changed the title ~~Django 1.11(b1) / python 3.6 support~~ Test against django 1.11(b1) / python 3.6 Mar 3, 2017

meshy added 2 commits March 3, 2017 22:16

Test against Python 3.6 and Django 1.11b1

a178ed7

meshy force-pushed the new-test-versions branch from a81787a to 8e52bdf Compare March 3, 2017 22:16

meshy commented Mar 3, 2017

View reviewed changes

Django 1.11 uses real class names in __repr__()

aefb7da

See https://code.djangoproject.com/ticket/27546 and django/django@48826aa

meshy force-pushed the new-test-versions branch from 0e4c684 to aefb7da Compare March 3, 2017 22:56

meshy commented Mar 3, 2017

View reviewed changes

meshy mentioned this pull request Mar 11, 2017

Only base classes provided in Django 1.11b1 #267

Closed

vdboor merged commit aefb7da into jazzband:master Apr 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test against django 1.11(b1) / python 3.6 #271

Test against django 1.11(b1) / python 3.6 #271

meshy commented Mar 2, 2017 •

edited

codecov-io commented Mar 2, 2017 •

edited

meshy commented Mar 2, 2017

meshy commented Mar 2, 2017

meshy commented Mar 2, 2017 •

edited

meshy commented Mar 3, 2017

meshy Mar 3, 2017

meshy Mar 3, 2017

meshy Mar 3, 2017

vdboor Apr 6, 2017 •

edited

meshy commented Mar 3, 2017 •

edited

timgraham commented Mar 10, 2017

francoisfreitag commented Mar 10, 2017 •

edited

meshy commented Mar 10, 2017

meshy commented Mar 10, 2017

francoisfreitag commented Mar 12, 2017 •

edited

vdboor commented Apr 6, 2017

meshy commented Apr 12, 2017

meshy commented Apr 12, 2017 •

edited

francoisfreitag commented Apr 19, 2017

Test against django 1.11(b1) / python 3.6 #271

Test against django 1.11(b1) / python 3.6 #271

Conversation

meshy commented Mar 2, 2017 • edited

codecov-io commented Mar 2, 2017 • edited

Codecov Report

meshy commented Mar 2, 2017

meshy commented Mar 2, 2017

meshy commented Mar 2, 2017 • edited

meshy commented Mar 3, 2017

meshy Mar 3, 2017

Choose a reason for hiding this comment

meshy Mar 3, 2017

Choose a reason for hiding this comment

meshy Mar 3, 2017

Choose a reason for hiding this comment

vdboor Apr 6, 2017 • edited

Choose a reason for hiding this comment

meshy commented Mar 3, 2017 • edited

timgraham commented Mar 10, 2017

francoisfreitag commented Mar 10, 2017 • edited

meshy commented Mar 10, 2017

meshy commented Mar 10, 2017

francoisfreitag commented Mar 12, 2017 • edited

vdboor commented Apr 6, 2017

meshy commented Apr 12, 2017

meshy commented Apr 12, 2017 • edited

francoisfreitag commented Apr 19, 2017

meshy commented Mar 2, 2017 •

edited

codecov-io commented Mar 2, 2017 •

edited

meshy commented Mar 2, 2017 •

edited

vdboor Apr 6, 2017 •

edited

meshy commented Mar 3, 2017 •

edited

francoisfreitag commented Mar 10, 2017 •

edited

francoisfreitag commented Mar 12, 2017 •

edited

meshy commented Apr 12, 2017 •

edited